* Re: [PATCH 3/5] revert: simplify getting commit subject in format_todo()
From: Jonathan Nieder @ 2011-12-07 7:06 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> format_todo() calls get_message(), but uses only the subject line of
> the commit message. Save work and unnecessary memory allocations by
> using find_commit_subject() instead. Also, remove the spurious check
> on cur->item: the previous patch makes sure that instruction sheets
> with missing commit subjects are parsable.
So, what effect will this patch actually have? Is it an optimization
with no other observable effect?
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Jonathan Nieder @ 2011-12-07 7:02 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Change the instruction sheet format subtly so that the subject of the
> commit message that follows the object name is optional. As a result,
> an instruction sheet like this is now perfectly valid:
>
> pick 35b0426
> pick fbd5bbcbc2e
> pick 7362160f
>
> While at it, also fix a bug: currently, we use a commit-id-shaped
> buffer to store the word after "pick" in '.git/sequencer/todo'. This
> is both wasteful and wrong because it places an artificial limit on
> the line length. Eliminate the need for the buffer altogether, and
> add a test demonstrating this.
>
> [jc: simplify parsing]
Reading the above does not make it at all obvious that I should want
to apply this patch because otherwise my prankster friend can cause my
copy of git to crash or run arbitrary code by putting a long commit
name in .git/sequencer/todo in our NFS-mounted shared checkout.
(Yes, I know there are other problems with such a setup, especially if
.git/hooks or .git/config is writable by untrusted people. So it is
not actually a security bug, but a robustness one.)
^ permalink raw reply
* Re: [PATCH 1/5] revert: free msg in format_todo()
From: Jonathan Nieder @ 2011-12-07 6:43 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Memory allocated to the fields of msg by get_message() isn't freed.
> This is potentially a big leak,
[because it is in a loop over commits being picked]
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> builtin/revert.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1ea525c..0c6d3d8 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> if (get_message(cur->item, &msg))
> return error(_("Cannot get commit message for %s"), sha1_abbrev);
> strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
> + free_message(&msg);
> }
> return 0;
> }
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-07 6:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h29fkfy.fsf@alter.siamese.dyndns.org>
On Tue, Dec 06, 2011 at 01:40:17PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ... You can now
> > do: "git credential-store erase </dev/null" to erase everything
> > (since you have provided no restrictions, it matches everything).
>
> That "justification" does not sound so true to me but perhaps that is
> because it is unclear what "erase" means and what it means to give the
> operation parameters.
It's not meant to be a justification, but rather an explanation. I think
the behavior is probably too dangerous to leave.
> When I see "erase $foo", I would find it natural if $foo meant "if there
> is something that matches $foo, then please remove it, but keep everything
> else intact", and not the other way around "Match the existing entries
> against a pattern (or a set of matching patterns) I am giving you, and
> drop all the rest". So if I happen to give you an empty set, I would
> expect nothing is removed.
It does do the first thing you mentioned (you provide one pattern $foo,
and we match the pattern you have given). It's just that the pattern you
have specified is "everything". The problem is not in the matching, but
in the pattern specification language.
This pattern:
protocol=https
host=github.com
means "match everything that uses https _and_ has a host of github.com".
The username and path fields are not present, which implicitly means
"don't care about them".
Similarly, this pattern:
protocol=https
means "match everything that uses https". Everything else is not
specified, and therefore we allow anything.
Then what does the empty pattern do? It cares about nothing, and
therefore matches everything.
By itself, I don't think that is a problem. It's something you might
want to specify, and it's logically consistent with the way the patterns
are matched. What is dangerous, though, is that failing to provide
input is byte-wise identical to the empty pattern. And that's why I say
it's a pattern specification problem.
A rough BNF for the pattern format is something like:
pattern = *line
line = key "=" value
key = *<any byte except NUL, "=", or "\n">
value = *<any byte except NUL or "\n">
Because the pattern takes 0 or more lines and no terminator, we can't
distinguish between empty or truncated input and the empty pattern. So
one solution would be:
pattern = *line "\n"
i.e., require a blank line terminator.
Does that explain the issue better?
-Peff
^ permalink raw reply
* [PATCH 5/5] revert: simplify communicating command-line arguments
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
From: Jonathan Nieder <jrnieder@gmail.com>
Currently, command-line arguments are communicated using (argc, argv)
until a prepare_revs() turns it into a terse structure. However,
since we plan to expose the cherry-picking machinery through a public
API in the future, we want callers to be able to call in with a
filled-in structure. For the revert builtin, this means that the
chief argument parser, parse_args(), should parse into such a
structure. Make this change.
[rr: minor improvements, commit message]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 53 +++++++++++++++++++++++++++++------------------------
1 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index f2bf198..3b25a0c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -59,13 +59,14 @@ struct replay_opts {
int allow_rerere_auto;
int mainline;
- int commit_argc;
- const char **commit_argv;
/* Merge strategy */
const char *strategy;
const char **xopts;
size_t xopts_nr, xopts_alloc;
+
+ /* Only used by REPLAY_NONE */
+ struct rev_info *revs;
};
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -168,9 +169,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
die(_("program error"));
}
- opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
- PARSE_OPT_KEEP_ARGV0 |
- PARSE_OPT_KEEP_UNKNOWN);
+ argc = parse_options(argc, argv, NULL, options, usage_str,
+ PARSE_OPT_KEEP_ARGV0 |
+ PARSE_OPT_KEEP_UNKNOWN);
/* Check for incompatible subcommands */
verify_opt_mutually_compatible(me,
@@ -212,9 +213,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
NULL);
}
- else if (opts->commit_argc < 2)
- usage_with_options(usage_str, options);
-
if (opts->allow_ff)
verify_opt_compatible(me, "--ff",
"--signoff", opts->signoff,
@@ -222,7 +220,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
"-x", opts->record_origin,
"--edit", opts->edit,
NULL);
- opts->commit_argv = argv;
+
+ if (opts->subcommand == REPLAY_NONE) {
+ opts->revs = xmalloc(sizeof(*opts->revs));
+ init_revisions(opts->revs, NULL);
+ opts->revs->no_walk = 1;
+ if (argc < 2)
+ usage_with_options(usage_str, options);
+ argc = setup_revisions(argc, argv, opts->revs, NULL);
+ } else
+ opts->revs = NULL;
+
+ /* Forbid stray command-line arguments */
+ if (argc > 1)
+ usage_with_options(usage_str, options);
}
struct commit_message {
@@ -631,23 +642,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,
return res;
}
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
{
- int argc;
-
- init_revisions(revs, NULL);
- revs->no_walk = 1;
if (opts->action != REPLAY_REVERT)
- revs->reverse = 1;
+ opts->revs->reverse ^= 1;
- argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
- if (argc > 1)
- usage(*revert_or_cherry_pick_usage(opts));
-
- if (prepare_revision_walk(revs))
+ if (prepare_revision_walk(opts->revs))
die(_("revision walk setup failed"));
- if (!revs->commits)
+ if (!opts->revs->commits)
die(_("empty commit set passed"));
}
@@ -837,14 +840,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
- struct rev_info revs;
struct commit *commit;
struct replay_insn_list **next;
- prepare_revs(&revs, opts);
+ prepare_revs(opts);
next = todo_list;
- while ((commit = get_revision(&revs)))
+ while ((commit = get_revision(opts->revs)))
next = replay_insn_list_append(opts->action, commit, next);
}
@@ -1037,6 +1039,9 @@ static int pick_revisions(struct replay_opts *opts)
struct replay_insn_list *todo_list = NULL;
unsigned char sha1[20];
+ if (opts->subcommand == REPLAY_NONE)
+ assert(opts->revs);
+
read_and_refresh_cache(opts);
/*
--
1.7.7.3
^ permalink raw reply related
* [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action. Now you can do:
pick fdc0b12 picked
revert 965fed4 anotherpick
For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.
This patch lays the foundation for extending the parser to support
more actions so 'git rebase -i' can reuse this machinery in the
future. While at it, also improve the error messages reported by the
insn sheet parser.
Helped-by: Jonathan Nieder <jrnider@gmail.com>
Acked-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 136 +++++++++++++++++++--------------------
sequencer.h | 8 ++
t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++
3 files changed, 133 insertions(+), 69 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 706b8d4..f2bf198 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-enum replay_action { REVERT, CHERRY_PICK };
enum replay_subcommand {
REPLAY_NONE,
REPLAY_REMOVE_STATE,
@@ -73,14 +72,14 @@ struct replay_opts {
static const char *action_name(const struct replay_opts *opts)
{
- return opts->action == REVERT ? "revert" : "cherry-pick";
+ return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
}
static char *get_encoding(const char *message);
static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
{
- return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+ return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
}
static int option_parse_x(const struct option *opt,
@@ -159,7 +158,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
OPT_END(),
};
- if (opts->action == CHERRY_PICK) {
+ if (opts->action == REPLAY_PICK) {
struct option cp_extra[] = {
OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,7 +362,7 @@ static int error_dirty_index(struct replay_opts *opts)
return error_resolve_conflict(action_name(opts));
/* Different translation strings for cherry-pick and revert */
- if (opts->action == CHERRY_PICK)
+ 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."));
@@ -467,7 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
return run_command_v_opt(args, RUN_GIT_CMD);
}
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+ struct replay_opts *opts)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -542,7 +542,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
defmsg = git_pathdup("MERGE_MSG");
- if (opts->action == REVERT) {
+ if (action == REPLAY_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -583,7 +583,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
@@ -607,13 +607,13 @@ 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 == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+ if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
- if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+ if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
write_cherry_pick_head(commit, "REVERT_HEAD");
if (res) {
- error(opts->action == REVERT
+ error(action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +637,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
init_revisions(revs, NULL);
revs->no_walk = 1;
- if (opts->action != REVERT)
+ if (opts->action != REPLAY_REVERT)
revs->reverse = 1;
argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
* assert(commit_list_count(list) == 2);
* return list;
*/
-static struct commit_list **commit_list_append(struct commit *commit,
- struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+ struct commit *operand,
+ struct replay_insn_list **next)
{
- struct commit_list *new = xmalloc(sizeof(struct commit_list));
- new->item = commit;
+ struct replay_insn_list *new = xmalloc(sizeof(*new));
+ new->action = action;
+ new->operand = operand;
*next = new;
new->next = NULL;
return &new->next;
}
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
- struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
{
- struct commit_list *cur = NULL;
- const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REVERT ? "revert" : "pick";
- const char *subject;
- int subject_len;
+ struct replay_insn_list *cur;
for (cur = todo_list; cur; cur = cur->next) {
- sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- subject_len = find_commit_subject(cur->item->buffer, &subject);
+ const char *sha1_abbrev, *action_str, *subject;
+ int subject_len;
+
+ action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ subject_len = find_commit_subject(cur->operand->buffer, &subject);
strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
subject_len, subject);
}
return 0;
}
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
unsigned char commit_sha1[20];
- enum replay_action action;
char *end_of_object_name;
int saved, status;
if (!prefixcmp(bol, "pick ")) {
- action = CHERRY_PICK;
+ item->action = REPLAY_PICK;
bol += strlen("pick ");
} else if (!prefixcmp(bol, "revert ")) {
- action = REVERT;
+ item->action = REPLAY_REVERT;
bol += strlen("revert ");
- } else
- return NULL;
+ } else {
+ size_t len = strchrnul(bol, '\n') - bol;
+ if (len > 255)
+ len = 255;
+ return error(_("Line %d: Unrecognized action: %.*s"),
+ lineno, (int)len, bol);
+ }
end_of_object_name = bol + strcspn(bol, " \n");
saved = *end_of_object_name;
@@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- const char *action_str;
- action_str = action == REVERT ? "revert" : "cherry-pick";
- error(_("Cannot %s during a %s"), action_str, action_name(opts));
- return NULL;
- }
-
if (status < 0)
- return NULL;
+ return error(_("Line %d: Malformed object name: %s"), lineno, bol);
- return lookup_commit_reference(commit_sha1);
+ item->operand = lookup_commit_reference(commit_sha1);
+ if (!item->operand)
+ return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
+
+ item->next = NULL;
+ return 0;
}
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
- struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
{
- struct commit_list **next = todo_list;
- struct commit *commit;
+ struct replay_insn_list **next = todo_list;
+ struct replay_insn_list item = {0, NULL, NULL};
char *p = buf;
int i;
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- commit = parse_insn_line(p, eol, opts);
- if (!commit)
- return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
+ if (parse_insn_line(p, eol, &item, i) < 0)
+ return -1;
+ next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
@@ -771,8 +769,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,
- struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
struct strbuf buf = STRBUF_INIT;
@@ -788,7 +785,7 @@ static void read_populate_todo(struct commit_list **todo_list,
}
close(fd);
- res = parse_insn_buffer(buf.buf, todo_list, opts);
+ res = parse_insn_buffer(buf.buf, todo_list);
strbuf_release(&buf);
if (res)
die(_("Unusable instruction sheet: %s"), todo_file);
@@ -837,18 +834,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
die(_("Malformed options sheet: %s"), opts_file);
}
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
- struct commit_list **next;
+ struct replay_insn_list **next;
prepare_revs(&revs, opts);
next = todo_list;
while ((commit = get_revision(&revs)))
- next = commit_list_append(commit, next);
+ next = replay_insn_list_append(opts->action, commit, next);
}
static int create_seq_dir(void)
@@ -946,7 +943,7 @@ fail:
return -1;
}
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
static struct lock_file todo_lock;
@@ -954,7 +951,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
int fd;
fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list, opts) < 0)
+ if (format_todo(&buf, todo_list) < 0)
die(_("Could not format %s."), todo_file);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
@@ -998,9 +995,10 @@ static void save_opts(struct replay_opts *opts)
}
}
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+ struct replay_opts *opts)
{
- struct commit_list *cur;
+ struct replay_insn_list *cur;
int res;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1010,8 +1008,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
read_and_refresh_cache(opts);
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur, opts);
- res = do_pick_commit(cur->item, opts);
+ save_todo(cur);
+ res = do_pick_commit(cur->operand, cur->action, opts);
if (res) {
if (!cur->next)
/*
@@ -1036,7 +1034,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
static int pick_revisions(struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
+ struct replay_insn_list *todo_list = NULL;
unsigned char sha1[20];
read_and_refresh_cache(opts);
@@ -1056,7 +1054,7 @@ static int pick_revisions(struct replay_opts *opts)
if (!file_exists(git_path(SEQ_TODO_FILE)))
return error(_("No %s in progress"), action_name(opts));
read_populate_opts(&opts);
- read_populate_todo(&todo_list, opts);
+ read_populate_todo(&todo_list);
/* Verify that the conflict has been resolved */
if (!index_differs_from("HEAD", 0))
@@ -1074,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)
if (create_seq_dir() < 0)
return -1;
if (get_sha1("HEAD", sha1)) {
- if (opts->action == REVERT)
+ if (opts->action == REPLAY_REVERT)
return error(_("Can't revert as initial commit"));
return error(_("Can't cherry-pick into empty head"));
}
@@ -1091,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
memset(&opts, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
- opts.action = REVERT;
+ opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
@@ -1106,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
int res;
memset(&opts, 0, sizeof(opts));
- opts.action = CHERRY_PICK;
+ opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index f435fdb..71cfcae 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,14 @@
#define SEQ_TODO_FILE "sequencer/todo"
#define SEQ_OPTS_FILE "sequencer/opts"
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+
+struct replay_insn_list {
+ enum replay_action action;
+ struct commit *operand;
+ struct replay_insn_list *next;
+};
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6390f2a..d52ad59 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
test_line_count = 4 commits
'
+test_expect_success 'revert --continue continues after cherry-pick' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ git revert --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'mixed pick and revert instructions' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ oldsha=`git rev-parse --short HEAD~1` &&
+ echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
1.7.7.3
^ permalink raw reply related
* [PATCH 3/5] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
format_todo() calls get_message(), but uses only the subject line of
the commit message. Save work and unnecessary memory allocations by
using find_commit_subject() instead. Also, remove the spurious check
on cur->item: the previous patch makes sure that instruction sheets
with missing commit subjects are parsable.
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..706b8d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
struct replay_opts *opts)
{
struct commit_list *cur = NULL;
- struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
const char *sha1_abbrev = NULL;
const char *action_str = opts->action == REVERT ? "revert" : "pick";
+ const char *subject;
+ int subject_len;
for (cur = todo_list; cur; cur = cur->next) {
sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- if (get_message(cur->item, &msg))
- return error(_("Cannot get commit message for %s"), sha1_abbrev);
- strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
- free_message(&msg);
+ subject_len = find_commit_subject(cur->item->buffer, &subject);
+ strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+ subject_len, subject);
}
return 0;
}
--
1.7.7.3
^ permalink raw reply related
* [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Change the instruction sheet format subtly so that the subject of the
commit message that follows the object name is optional. As a result,
an instruction sheet like this is now perfectly valid:
pick 35b0426
pick fbd5bbcbc2e
pick 7362160f
While at it, also fix a bug: currently, we use a commit-id-shaped
buffer to store the word after "pick" in '.git/sequencer/todo'. This
is both wasteful and wrong because it places an artificial limit on
the line length. Eliminate the need for the buffer altogether, and
add a test demonstrating this.
[jc: simplify parsing]
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 37 ++++++++++++++++---------------------
t/t3510-cherry-pick-sequence.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
return 0;
}
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
{
unsigned char commit_sha1[20];
- char sha1_abbrev[40];
enum replay_action action;
- int insn_len = 0;
- char *p, *q;
+ char *end_of_object_name;
+ int saved, status;
- if (!prefixcmp(start, "pick ")) {
+ if (!prefixcmp(bol, "pick ")) {
action = CHERRY_PICK;
- insn_len = strlen("pick");
- p = start + insn_len + 1;
- } else if (!prefixcmp(start, "revert ")) {
+ bol += strlen("pick ");
+ } else if (!prefixcmp(bol, "revert ")) {
action = REVERT;
- insn_len = strlen("revert");
- p = start + insn_len + 1;
+ bol += strlen("revert ");
} else
return NULL;
- q = strchr(p, ' ');
- if (!q)
- return NULL;
- q++;
-
- strlcpy(sha1_abbrev, p, q - p);
+ end_of_object_name = bol + strcspn(bol, " \n");
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ status = get_sha1(bol, commit_sha1);
+ *end_of_object_name = saved;
/*
* Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
return NULL;
}
- if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+ if (status < 0)
return NULL;
return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
int i;
for (i = 1; *p; i++) {
- commit = parse_insn_line(p, opts);
+ char *eol = strchrnul(p, '\n');
+ commit = parse_insn_line(p, eol, opts);
if (!commit)
return error(_("Could not parse line %d."), i);
next = commit_list_append(commit, next);
- p = strchrnul(p, '\n');
- if (*p)
- p++;
+ p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features
. ./test-lib.sh
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
pristine_detach () {
git cherry-pick --quit &&
git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '
test_must_fail git cherry-pick --continue
'
+test_expect_success 'malformed instruction sheet 3' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "resolved" >foo &&
+ git add foo &&
+ git commit &&
+ sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ test_must_fail git cherry-pick --continue
+'
+
+test_expect_success 'commit descriptions in insn sheet are optional' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ git rev-list HEAD >commits &&
+ test_line_count = 4 commits
+'
+
test_done
--
1.7.7.3
^ permalink raw reply related
* [PATCH 1/5] revert: free msg in format_todo()
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit. Fix this using
free_message().
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
if (get_message(cur->item, &msg))
return error(_("Cannot get commit message for %s"), sha1_abbrev);
strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+ free_message(&msg);
}
return 0;
}
--
1.7.7.3
^ permalink raw reply related
* [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
Hi,
This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
($gname/186365) implemented. My new branch rr/sequencer will be based
on this branch.
Thanks.
Jonathan Nieder (1):
revert: simplify communicating command-line arguments
Ramkumar Ramachandra (4):
revert: free msg in format_todo()
revert: make commit subjects in insn sheet optional
revert: simplify getting commit subject in format_todo()
revert: allow mixed pick and revert instructions
builtin/revert.c | 225 +++++++++++++++++++--------------------
sequencer.h | 8 ++
t/t3510-cherry-pick-sequence.sh | 86 +++++++++++++++
3 files changed, 206 insertions(+), 113 deletions(-)
--
1.7.7.3
^ permalink raw reply
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-07 5:45 UTC (permalink / raw)
To: Björn Steinbrink
Cc: Vijay Lakshminarayanan, Junio C Hamano, git@vger.kernel.org,
Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <20111207045325.GA22990@atjola.homenet>
On 12/7/2011 10:23 AM, Björn Steinbrink wrote:
> That looks like you did something like:
> export GIT_EDITOR="cat git commit --amend"
>
> But the original command was:
> GIT_EDITOR=cat git commit --amend
>
> Notice that there are no quotes and no escaped spaces. This is a
> shortcut to set GIT_EDITOR to "cat" for just this one command (git
> commit --amend).
>
> If you want to set the editor in the environment, use just "export
> GIT_EDITOR=cat" or something like that.
Ok. Got it now.
Now, whats the benefit of
GIT_EDITOR=cat git commit --amend
over
git commit --amend -C HEAD
?
Obviously if we have more than one commit to handle during a rebase then,
setting editor to cat once, would be good. As now we don't really need to
do git commit --amend. We can add commits and continue rebase.
But for single commit probably second one looks easier. Isn't it?
Or maybe the latest patch from Junio is even better.
--
viresh
^ permalink raw reply
* Re: Query on git commit amend
From: Björn Steinbrink @ 2011-12-07 4:53 UTC (permalink / raw)
To: Viresh Kumar
Cc: Vijay Lakshminarayanan, Junio C Hamano, git@vger.kernel.org,
Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <4EDEE988.2070902@st.com>
On 2011.12.07 09:50:24 +0530, Viresh Kumar wrote:
>
> Thanks guys. This whole session was new to me.
>
> On 12/7/2011 7:58 AM, Vijay Lakshminarayanan wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> > Vijay Lakshminarayanan <laksvij@gmail.com> writes:
> >> >
> >>> >> I've found
> >>> >>
> >>> >> $ GIT_EDITOR=cat git commit --amend
> >>> >>
> >>> >> useful.
> >> >
> >> > Are you sure it is a cat?
> > Yes.
>
> This didn't worked for me. Got following error:
>
> cat: unrecognized option `--amend'
> Try `cat --help' for more information.
> error: There was a problem with the editor 'cat git commit --amend'.
> Please supply the message using either -m or -F option.
> Could not commit staged changes.
That looks like you did something like:
export GIT_EDITOR="cat git commit --amend"
But the original command was:
GIT_EDITOR=cat git commit --amend
Notice that there are no quotes and no escaped spaces. This is a
shortcut to set GIT_EDITOR to "cat" for just this one command (git
commit --amend).
If you want to set the editor in the environment, use just "export
GIT_EDITOR=cat" or something like that.
HTH
Björn
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: Jeff King @ 2011-12-07 4:42 UTC (permalink / raw)
To: René Scharfe; +Cc: Thomas Rast, git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9ED1.8010502@lsrfire.ath.cx>
On Wed, Dec 07, 2011 at 12:01:37AM +0100, René Scharfe wrote:
> Reading of git objects needs to be protected by an exclusive lock
> and cannot be parallelized. Searching the read buffers can be done
> in parallel, but for simple expressions threading is a net loss due
> to its overhead, as measured by Thomas. Turn it off unless we're
> searching in the worktree.
Based on my earlier numbers, I was going to complain that we should
also be checking the "simple expressions" assumption here, as time spent
in the actual regex might be important.
However, after trying to repeat my experiment, I think the numbers I
posted earlier were misleading. For example, using my "more complex"
regex of 'a.*b':
$ time git grep --threads=8 'a.*b' HEAD >/dev/null
real 0m8.655s
user 0m23.817s
sys 0m0.480s
Look at that sweet, sweet parallelism. It's a quad-core with
hyperthreading, so we're not getting the 8x speedup we might hope for
(presumably due to lock contention on extracting blobs), but hey, 3x
isn't bad. Except, wait:
$ time git grep --threads=0 'a.*b' HEAD >/dev/null
real 0m7.651s
user 0m7.600s
sys 0m0.048s
We can get 1x on a single core, but the total time is lower! This
processor is an i7 with "turbo boost", which means it clocks higher in
single-core mode than when multiple cores are active. So the numbers I
posted earlier were misleading. Yes, we got parallelism, but at the cost
of knocking the clock speed down for a net loss.
The sweet spot for me seems to be:
$ time git grep --threads=2 'a.*b' HEAD >/dev/null
real 0m6.303s
user 0m11.129s
sys 0m0.220s
I'd be curious to see results from somebody with a quad-core (or more)
without turbo boost; I suspect that threading may have more benefit
there, even though we have some lock contention for blobs.
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1048,7 +1048,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> nr_threads = 0;
> #else
> if (nr_threads == -1)
> - nr_threads = (online_cpus() > 1) ? THREADS : 0;
> + nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
>
> if (nr_threads > 0) {
> opt.use_threads = 1;
This doesn't kick in for "--cached", which has the same performance
characteristics as grepping a tree. I think you want to add "&& !cached" to
the conditional.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: Jeff King @ 2011-12-07 4:24 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
On Tue, Dec 06, 2011 at 11:48:26PM +0100, René Scharfe wrote:
> #ifndef NO_PTHREADS
> - if (use_threads) {
> + if (nr_threads > 0) {
> grep_sha1_async(opt, name, sha1);
> return 0;
> } else
Should this be "if (nr_threads > 1)"?
As a user, I would do:
git grep --threads=1 ...
if I wanted a single-threaded process. Instead, we actually spawn a
sub-thread and do all of the locking, which has a measurable cost:
$ time git grep --threads=0 SIMPLE HEAD >/dev/null
real 0m2.994s
user 0m2.932s
sys 0m0.060s
$ time git grep --threads=1 SIMPLE HEAD >/dev/null
real 0m3.407s
user 0m3.392s
sys 0m0.140s
Should --threads=1 be equivalent to --threads=0?
-Peff
^ permalink raw reply
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-07 4:20 UTC (permalink / raw)
To: Vijay Lakshminarayanan, Junio C Hamano
Cc: git@vger.kernel.org, Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <87wra9und4.fsf@gmail.com>
Thanks guys. This whole session was new to me.
On 12/7/2011 7:58 AM, Vijay Lakshminarayanan wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> > Vijay Lakshminarayanan <laksvij@gmail.com> writes:
>> >
>>> >> I've found
>>> >>
>>> >> $ GIT_EDITOR=cat git commit --amend
>>> >>
>>> >> useful.
>> >
>> > Are you sure it is a cat?
> Yes.
This didn't worked for me. Got following error:
cat: unrecognized option `--amend'
Try `cat --help' for more information.
error: There was a problem with the editor 'cat git commit --amend'.
Please supply the message using either -m or -F option.
Could not commit staged changes.
>> > I almost always use
>> >
>> > $ EDITOR=: git commit --amend
Even this didn't worked for me:
error: pathspec '.git/COMMIT_EDITMSG' did not match any file(s) known to git.
error: There was a problem with the editor 'git commit --amend'.
Please supply the message using either -m or -F option.
Could not commit staged changes.
Only "true" worked for me.
Probably, i have an older version of git (version 1.7.2.2)
One more thing. I couldn't get completely how this worked. Maybe any pointers to
earlier discussions.
The way i am testing it is:
- Stop after a commit in middle of rebase using "edit" or "e" option
- set EDITOR or GIT_EDITOR
- change files
- git add changed_files
- git rebase --continue
--
viresh
^ permalink raw reply
* [PATCH, v5] git-tag: introduce --cleanup option
From: Kirill A. Shutemov @ 2011-12-07 3:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Kirill A. Shutemov
From: "Kirill A. Shutemov" <kirill@shutemov.name>
Normally git tag strips tag message lines starting with '#', trailing
spaces from every line and empty lines from the beginning and end.
--cleanup allows to select different cleanup modes for tag message.
It provides the same interface as --cleanup option in git-commit.
Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
Documentation/git-tag.txt | 7 +++++
builtin/tag.c | 67 +++++++++++++++++++++++++++++++++++---------
2 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index c83cb13..622a019 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,6 +99,13 @@ OPTIONS
Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
is given.
+--cleanup=<mode>::
+ This option sets how the tag message is cleaned up.
+ The '<mode>' can be one of 'verbatim', 'whitespace' and 'strip'. The
+ 'strip' mode is default. The 'verbatim' mode does not change message at
+ all, 'whitespace' removes just leading/trailing whitespace lines and
+ 'strip' removes both whitespace and commentary.
+
<tagname>::
The name of the tag to create, delete, or describe.
The new tag name must pass all checks defined by
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..51a4184 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -268,6 +268,15 @@ static const char tag_template[] =
N_("\n"
"#\n"
"# Write a tag message\n"
+ "# Lines starting with '#' will be ignored.\n"
+ "#\n");
+
+static const char tag_template_nocleanup[] =
+ N_("\n"
+ "#\n"
+ "# Write a tag message\n"
+ "# Lines starting with '#' will be kept; you may remove them"
+ " yourself if you want to.\n"
"#\n");
static void set_signingkey(const char *value)
@@ -319,8 +328,18 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
return 0;
}
+struct create_tag_options {
+ unsigned int message_given:1;
+ unsigned int sign;
+ enum {
+ CLEANUP_NONE,
+ CLEANUP_SPACE,
+ CLEANUP_ALL
+ } cleanup_mode;
+};
+
static void create_tag(const unsigned char *object, const char *tag,
- struct strbuf *buf, int message, int sign,
+ struct strbuf *buf, struct create_tag_options *opt,
unsigned char *prev, unsigned char *result)
{
enum object_type type;
@@ -345,7 +364,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (header_len > sizeof(header_buf) - 1)
die(_("tag header too big."));
- if (!message) {
+ if (!opt->message_given) {
int fd;
/* write the template message before editing: */
@@ -356,8 +375,12 @@ static void create_tag(const unsigned char *object, const char *tag,
if (!is_null_sha1(prev))
write_tag_body(fd, prev);
+ else if (opt->cleanup_mode == CLEANUP_ALL)
+ write_or_die(fd, _(tag_template),
+ strlen(_(tag_template)));
else
- write_or_die(fd, _(tag_template), strlen(_(tag_template)));
+ write_or_die(fd, _(tag_template_nocleanup),
+ strlen(_(tag_template_nocleanup)));
close(fd);
if (launch_editor(path, buf, NULL)) {
@@ -367,14 +390,15 @@ static void create_tag(const unsigned char *object, const char *tag,
}
}
- stripspace(buf, 1);
+ if (opt->cleanup_mode != CLEANUP_NONE)
+ stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
- if (!message && !buf->len)
+ if (!opt->message_given && !buf->len)
die(_("no tag message?"));
strbuf_insert(buf, 0, header_buf, header_len);
- if (build_tag_object(buf, sign, result) < 0) {
+ if (build_tag_object(buf, opt->sign, result) < 0) {
if (path)
fprintf(stderr, _("The tag message has been left in %s\n"),
path);
@@ -422,9 +446,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
unsigned char object[20], prev[20];
const char *object_ref, *tag;
struct ref_lock *lock;
-
- int annotate = 0, sign = 0, force = 0, lines = -1,
- list = 0, delete = 0, verify = 0;
+ struct create_tag_options opt;
+ char *cleanup_arg = NULL;
+ int annotate = 0, force = 0, lines = -1, list = 0,
+ delete = 0, verify = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -442,7 +467,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", &msg, "message",
"tag message", parse_msg_arg),
OPT_FILENAME('F', "file", &msgfile, "read message from file"),
- OPT_BOOLEAN('s', "sign", &sign, "annotated and GPG-signed tag"),
+ OPT_BOOLEAN('s', "sign", &opt.sign, "annotated and GPG-signed tag"),
+ OPT_STRING(0, "cleanup", &cleanup_arg, "mode",
+ "how to strip spaces and #comments from message"),
OPT_STRING('u', "local-user", &keyid, "key-id",
"use another key to sign the tag"),
OPT__FORCE(&force, "replace the tag if exists"),
@@ -459,13 +486,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
git_config(git_tag_config, NULL);
+ memset(&opt, 0, sizeof(opt));
+
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
if (keyid) {
- sign = 1;
+ opt.sign = 1;
set_signingkey(keyid);
}
- if (sign)
+ if (opt.sign)
annotate = 1;
if (argc == 0 && !(delete || verify))
list = 1;
@@ -523,9 +552,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
else if (!force)
die(_("tag '%s' already exists"), tag);
+ opt.message_given = msg.given || msgfile;
+
+ if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
+ opt.cleanup_mode = CLEANUP_ALL;
+ else if (!strcmp(cleanup_arg, "verbatim"))
+ opt.cleanup_mode = CLEANUP_NONE;
+ else if (!strcmp(cleanup_arg, "whitespace"))
+ opt.cleanup_mode = CLEANUP_SPACE;
+ else
+ die(_("Invalid cleanup mode %s"), cleanup_arg);
+
if (annotate)
- create_tag(object, tag, &buf, msg.given || msgfile,
- sign, prev, object);
+ create_tag(object, tag, &buf, &opt, prev, object);
lock = lock_any_ref_for_update(ref.buf, prev, 0);
if (!lock)
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH, v4] git-tag: introduce --cleanup option
From: Kirill A. Shutemov @ 2011-12-07 2:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vvcpuk5ex.fsf@alter.siamese.dyndns.org>
On Mon, Dec 05, 2011 at 02:41:26PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > More importantly, though, this seems to break t6300 badly. I haven't
> > looked into why yet, though.
>
> Probably two issues.
>
> - opt.message (and the original 'message') was misnamed and confused the
> patch author what "if (!message && !buf->len)" meant.
>
> - "opt" is a structure meant to be extensible, but is not initialized as
> a whole, inviting future errors.
Sorry for that and thanks for the investigation.
> It still seems to be broken with respect to the primary thing the patch
> wanted to do (t7400 "git tag -F commentsfile comments-annotated-tag" does
> not seem to produce an expected result), so I'll kick it back to the
> Kirill to look at.
CLEANUP_ALL should always be default regardless of opt.message_given.
I'll send new version.
--
Kirill A. Shutemov
^ permalink raw reply
* Re: Query on git commit amend
From: Vijay Lakshminarayanan @ 2011-12-07 2:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Viresh Kumar, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <7vobvlfowk.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Vijay Lakshminarayanan <laksvij@gmail.com> writes:
>
>> I've found
>>
>> $ GIT_EDITOR=cat git commit --amend
>>
>> useful.
>
> Are you sure it is a cat?
Yes.
Did you mean something else by your question?
> I almost always use
>
> $ EDITOR=: git commit --amend
I just tried this out. This and Peff's GIT_EDITOR=true silently dwiw
but cat is useful to review the commit.
--
Cheers
~vijay
Gnus should be more complicated.
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Junio C Hamano @ 2011-12-07 2:28 UTC (permalink / raw)
To: Michael Schubert; +Cc: Joseph Huttner, git
In-Reply-To: <4EDEA2E2.3030002@elegosoft.com>
Michael Schubert <mschub@elegosoft.com> writes:
>> What are your thoughts?
>
> If it's no social issue but just due to lack of a reminder you
> could provide a template for commit.template. Either way: you
> still would have to force people to set it.?
While that would be a good first step, I think people will learn best when
they feel by their skin how good log messages help them in the long run.
Pick a recent bugfix in your project, analyze why the code was broken by
the bug in the first place, and view the log message of the commit that
introduced the code that was broken by the buggy commit. You will often
notice that the original commit did not explain why the code needs to be
that way sufficiently, risking later breakage, and the buggy commit that
broke the code did not justify the change any more than "This happens to
make something work for me in a particular narrow case".
And then look at the log message of the bugfix. Does it explain why the
broken change was bad, and the fixed code _has to be_ that way?
Do this for a handful of examples, and you will start noticing patterns,
and what makes good messages that become useful in the longer term. Have
your people learn from good ones _as well as_ the bad ones.
Have fun.
^ permalink raw reply
* Re: Query on git commit amend
From: Vijay Lakshminarayanan @ 2011-12-07 2:18 UTC (permalink / raw)
To: Jeff King; +Cc: Viresh Kumar, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <20111206191124.GE9492@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 06, 2011 at 09:16:18PM +0530, Vijay Lakshminarayanan wrote:
>
>> I've found
>>
>> $ GIT_EDITOR=cat git commit --amend
>>
>> useful.
>>
>> The benefit of this technique is that it even works for git-rebase -i.
>
> I sometimes do a similar thing, but I don't use "cat". That will dump
> all of the log message (including the generated template) to stdout
> (i.e., the terminal), which is quite noisy. Instead, I use:
>
> GIT_EDITOR=true git commit --amend
>
> which silently leaves the file untouched.
Thanks Peff. I didn't know about true. I will use it when rebasing.
cat's noisiness is useful as a review of the output.
> -Peff
--
Cheers
~vijay
Gnus should be more complicated.
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Nguyen Thai Ngoc Duy @ 2011-12-07 2:08 UTC (permalink / raw)
To: Joshua Redstone
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CB04005C.2C669%joshua.redstone@fb.com>
On Wed, Dec 7, 2011 at 8:48 AM, Joshua Redstone <joshua.redstone@fb.com> wrote:
> I tried doing a 'git read-tree HEAD' before each 'git add ; git commit'
> iteration, and the time for git-commit jumped from about 1 second to about
> 8 seconds. That is a pretty dramatic slowdown. Any idea why? I wonder
> if that's related to the overall commit slowness.
How big is your working directory? "git ls-files | wc -l" should show
it. Try "git read-tree HEAD; git add; git write-tree" and see if the
write-tree part takes as much time as commit. write-tree is mainly
about cache-tree generation.
> @Carlos and/or @Junio, can you point me at any docs/code to understand
> what a tree-cache is and how it differs from the index? I did a google
> search for [git tree-cache index], but nothing popped out.
Have a look at Documentation/technical/index-format.txt. Cache tree
extension is near the end.
--
Duy
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-07 1:48 UTC (permalink / raw)
To: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano
Cc: git@vger.kernel.org
In-Reply-To: <20111203002347.GB2950@centaur.lab.cmartin.tk>
Hi Carlos and Tomas and Junio,
@Tomas, I tried adding the '--no-status' flag to 'git commit' and it sped
things up by maybe 15%, but commits still take a second.
@Carlos, by "same size", I mean roughly the same number of files and
number of bytes modified in each file. In all experiments, it's less than
5 files modified per commit with changes totaling fewer than 10 KB, often
more like 1 KB. I actually wrote a test script to generate commits,
customized for the stats on the repo I'm using. It repeatedly generates
some changes, does 'git add [ list of files changed ]' followed by 'git
commit --no-status -m [ msg ]'. It generates changes by picking fewer
than 5 files at random, modifying two 100-byte regions in each file, and
occasionally creates a new file of about 1 KB. If it helps, I can
probably post the test script I've been using.
I tried doing a 'git read-tree HEAD' before each 'git add ; git commit'
iteration, and the time for git-commit jumped from about 1 second to about
8 seconds. That is a pretty dramatic slowdown. Any idea why? I wonder
if that's related to the overall commit slowness.
@Carlos and/or @Junio, can you point me at any docs/code to understand
what a tree-cache is and how it differs from the index? I did a google
search for [git tree-cache index], but nothing popped out.
Cheers,
Josh
On 12/2/11 4:23 PM, "Carlos Martín Nieto" <cmn@elego.de> wrote:
>On Fri, Dec 02, 2011 at 11:17:10PM +0000, Joshua Redstone wrote:
>> Hi,
>> I have a git repo with about 300k commits, 150k files totaling maybe
>>7GB.
>> Locally committing a small change - say touching fewer than 300 bytes
>> across 4 files - consistently takes over one second, which seems kinda
>> slow. This is using git 1.7.7.4 on a linux 2.6 box. The time does not
>> improve after doing a git-gc (my .git dir has maybe 250 files after a
>>git
>> gc). The same size commit on a brand new repo takes < 10ms. Any
>>thoughts
>> on why committing a small change seems to take a long time on larger
>>repos?
>
>By "same size commit" do you mean the same amount of changes, or the
>same amount of files? Committing doesn't depend on the size of the
>repo (by itself), but on the size of the index, which depends on the
>amount of files to be committed (as git is snapshot-based). At one
>point, commit forgot how to write the tree cache to the index (a
>performance optimisation). Do the times improve if you run 'git
>read-tree HEAD' between one commit and another? Note that this will
>reset the index to the last commit, though for the tests I image you
>use some variation of 'git commit -a'.
>
>Thomas Rast wrote a patch to re-teach commit to store the tree cache,
>but there were some issues and never got applied.
>
>>
>> Fwiw, I also tried doing the same test using libgit2 (via the pygit2
>> wrapper), and it was ever slower (about 6 seconds to commit the same
>>small
>> change).
>
>I don't know about the python bindings, but on the (somewhat
>unscientific) tests for libgit2's write-tree (the slow part of a
>creating a commit), it performs slightly faster than git's (though I
>think git's write-tree does update the tree cache, which libgit2
>doesn't currently). The speed could just be a side-effect of the small
>test repo. From your domain, I assume the data is not for public
>consumption, but it'd be great if you could post your code to pygit2's
>issue tracker so we can see how much of the slowdown comes from the
>bindings or the library.
>
> cmn
>
^ permalink raw reply
* Re: Suggestion on hashing
From: Bill Zaumen @ 2011-12-07 1:44 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Chris West (Faux), Jeff King, Git Mailing List
In-Reply-To: <CACsJy8CXkz-W3Z3pX-C-+fjLz=WahBajE2uLEG-f3gG_svEhug@mail.gmail.com>
On Tue, 2011-12-06 at 13:23 +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Dec 6, 2011 at 1:02 PM, Bill Zaumen <bill.zaumen@gmail.com> wrote:
> > If you are replacing SHA-1 as an object ID with another hash function,
> > two things to watch are submodules and alternative object databases.
> > Because of those, it is necessary to worry about the order in which
> > repositories are converted. In the worst case for submodules, you'd
> > have to do multiple repositories at the same time, switching between
> > them depending on what you need at each point.
>
> I know migration would be painful. But note that new repos can benefit
> stronger digest without legacy (of course until it links to an old
> repo). For submodules, I think we should extend it to become something
> similar to soft-link: git link is an SHA-1 to a text file that
> contains SHA-1 and maybe other digests of the submodule's tip.
Repositories would need to store a table mapping old SHA-1 values to
the new ones (for commits). There's nothing in a repository to
reliably indicate that it is being used as a submodule, and the choice
of submodules can vary from commit to commit, making it difficult to
control the order in which objects have their hashes updated. In some
corner cases, you could have two branches in each of two repositories
with different choices as to which is a submodule of which, although
I'd be surprised if anyone actually did that.
Aside from that, in some corporate environments, the IT departments
want to determine the release schedule for applications, and would
take a dim view of changes that could not be tested first without being
widely deployed. You could end up making Git unacceptable for those
departments if you do not maintain backwards compatibility with
existing repositories.
^ permalink raw reply
* Re: [PATCHv2 06/13] credential: apply helper config
From: Jeff King @ 2011-12-07 0:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20111207004511.GA28370@sigill.intra.peff.net>
On Tue, Dec 06, 2011 at 07:45:11PM -0500, Jeff King wrote:
> On Tue, Dec 06, 2011 at 03:58:35PM -0800, Junio C Hamano wrote:
>
> > > +test_expect_success 'respect configured credentials' '
> > > + test_config credential.helper "$HELPER" &&
> > > + check fill <<-\EOF
> > > + --
> > > + username=foo
> > > + password=bar
> > > + --
> > > + EOF
> > > +'
> >
> > Hmm, why do I get ask-ass-{username,password} from this one?
>
> Ugh. Because apparently one of my re-roll tweaks from patch 03 regressed
> this. Sorry, I should have been more careful about running the full
> suite, not just the tests in the commits I tweaked.
>
> Let me investigate and get back to you.
Brown paper bag time.
This needs squashed in, due to the changes in patch 03/13:
---
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index e3f61f4..42d0f5b 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -192,7 +192,7 @@ test_expect_success 'internal getpass does not ask for known username' '
EOF
'
-HELPER="f() {
+HELPER="!f() {
cat >/dev/null
echo username=foo
echo password=bar
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 21e2f5d..c59908f 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -102,7 +102,7 @@ test_expect_success 'http auth can request both user and pass' '
'
test_expect_success 'http auth respects credential helper config' '
- test_config_global credential.helper "f() {
+ test_config_global credential.helper "!f() {
cat >/dev/null
echo username=user@host
echo password=user@host
^ permalink raw reply related
* Re: [PATCHv2 06/13] credential: apply helper config
From: Jeff King @ 2011-12-07 0:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsjkxckwk.fsf@alter.siamese.dyndns.org>
On Tue, Dec 06, 2011 at 03:58:35PM -0800, Junio C Hamano wrote:
> > +test_expect_success 'respect configured credentials' '
> > + test_config credential.helper "$HELPER" &&
> > + check fill <<-\EOF
> > + --
> > + username=foo
> > + password=bar
> > + --
> > + EOF
> > +'
>
> Hmm, why do I get ask-ass-{username,password} from this one?
Ugh. Because apparently one of my re-roll tweaks from patch 03 regressed
this. Sorry, I should have been more careful about running the full
suite, not just the tests in the commits I tweaked.
Let me investigate and get back to you.
-Peff
^ 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