* [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
* Re: [RFD] Handling of non-UTF8 data in gitweb
From: Junio C Hamano @ 2011-12-07 0:37 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Jürgen Kreileder, John Hawley
In-Reply-To: <201112041709.32212.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> But doing this would change gitweb behavior. Currently when
> encountering something (usually line of output) that is not valid
> UTF-8, we decode it (to UTF-8) using $fallback_encoding, by default
> 'latin1'. Note however that this value is per gitweb installation,
> not per repository.
I think we added and you acked 00f429a (gitweb: Handle non UTF-8 text
better, 2007-06-03) for a good reason, and I think the above argues that
whatever issue the commit tried to address is a non-issue. Is it really
true?
> ... I guess
> it could be emulated by defining our own 'utf-8-with-fallback'
> encoding, or by defining our own PerlIO layer with PerlIO::via.
> But it no longer be simple solution (though still automatic).
Between the current "everybody who read from the input must remember to
call to_utf8" and "everybody gets to read utf8 automatically for internal
processing", even though the latter may involve one-time pain to set up
the framework to do so, the pros-and-cons feels obvious to me.
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Hilco Wijbenga @ 2011-12-07 0:08 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git, Michael Schubert
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
On 6 December 2011 14:55, Joseph Huttner <huttnified@gmail.com> wrote:
> So I know that there is a somewhat standard format of commit messages
> in Git, which Linus outlines here:
>
> https://github.com/torvalds/subsurface/blob/master/README#L164
>
> Trouble is, when most people go to commit, the file that the editor
> opens has no reminder of how to write a proper commit message. Often
> I find myself having to go back through the commit log, or consulting
> the above link.
>
> I propose two things:
>
> 1. An optional flag in the Git config that, if set, shows the format
> of a typical commit message in your commit message template.
>
> 2. The ability to modify this commit message template. Many teams
> use automated tools to read commit messages and then do automated
> tasks based on that data, like comment an RT ticket. Thus, developers
> need to be reminded of these team-specific settings as well.
>
> What are your thoughts?
Great idea! These templates would be stored in the Git repo, I assume?
Btw, there is 'commit.template' which you can use locally.
I was wondering if it might be possible to somehow add project config
defaults to one's Git repo. It would be great to have something like
'commit.template' point to a file in the Git repo by default.
Currently, it doesn't seem possible to have a config parameter "point
to" a file or directory in the Git repo. Nor do I know of a way to
have the Git repo set a config parameter to a default value. Or is
this possible after all?
^ permalink raw reply
* Re: [PATCHv2 06/13] credential: apply helper config
From: Junio C Hamano @ 2011-12-06 23:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062247.GF29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 81a455f..e3f61f4 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -192,4 +192,46 @@ test_expect_success 'internal getpass does not ask for known username' '
> EOF
> '
>
> +HELPER="f() {
> + cat >/dev/null
> + echo username=foo
> + echo password=bar
> + }; f"
> +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?
> +test_expect_success 'match configured credential' '
> + test_config credential.https://example.com.helper "$HELPER" &&
> + check fill <<-\EOF
> + protocol=https
> + host=example.com
> + path=repo.git
> + --
> + username=foo
> + password=bar
> + --
> + EOF
> +'
And this one, too...
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Michael Schubert @ 2011-12-06 23:18 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
On 12/06/2011 11:55 PM, Joseph Huttner wrote:
> So I know that there is a somewhat standard format of commit messages
> in Git, which Linus outlines here:
>
> https://github.com/torvalds/subsurface/blob/master/README#L164
>
> Trouble is, when most people go to commit, the file that the editor
> opens has no reminder of how to write a proper commit message. Often
> I find myself having to go back through the commit log, or consulting
> the above link.
>
> I propose two things:
>
> 1. An optional flag in the Git config that, if set, shows the format
> of a typical commit message in your commit message template.
>
> 2. The ability to modify this commit message template. Many teams
> use automated tools to read commit messages and then do automated
> tasks based on that data, like comment an RT ticket. Thus, developers
> need to be reminded of these team-specific settings as well.
>
> 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.?
^ permalink raw reply
* Re: [PATCH 5/5] reset: update cache-tree data when appropriate
From: Junio C Hamano @ 2011-12-06 23:13 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Carlos Martín Nieto
In-Reply-To: <1385c10084ae41ae4543ef3ccaa1d6c8182b2204.1323191497.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> In the case of --mixed and --hard, we throw away the old index and
> rebuild everything from the tree argument (or HEAD). So we have an
> opportunity here to fill in the cache-tree data, just as read-tree
> did.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
> builtin/reset.c | 7 +++++++
> t/t0090-cache-tree.sh | 4 ++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 811e8e2..8c2c1d5 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -43,6 +43,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> int nr = 1;
> int newfd;
> struct tree_desc desc[2];
> + struct tree *tree;
> struct unpack_trees_options opts;
> struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
>
> @@ -84,6 +85,12 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> return error(_("Failed to find tree of %s."), sha1_to_hex(sha1));
> if (unpack_trees(nr, desc, &opts))
> return -1;
> +
> + if (reset_type == MIXED || reset_type == HARD) {
> + tree = parse_tree_indirect(sha1);
> + prime_cache_tree(&active_cache_tree, tree);
> + }
The basic idea that MIXED or HARD should result in a cache-tree that match
the tree we just read is sound, but how expensive is prime_cache_tree()? I
think it reads the same tree once again. Admittedly, the data needed to
reconstruct the tree is likely to be hot in core, but it may be necessary
to measure before deciding if this is a good change.
^ permalink raw reply
* [PATCH 4/2] grep: turn off threading for non-worktree
From: René Scharfe @ 2011-12-06 23:01 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
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.
Once the object store can be read safely by multiple threads in
parallel this patch should be reverted.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Goes on top of my earlier patch. Could use a better commit message
with your (cleaned up) performance numbers.
Documentation/git-grep.txt | 5 +++--
builtin/grep.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 47ac188..e981a9b 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -228,8 +228,9 @@ OPTIONS
there is a match and with non-zero status when there isn't.
--threads <n>::
- Run <n> search threads in parallel. Default is 8. This option
- is ignored if git was built without support for POSIX threads.
+ Run <n> search threads in parallel. Default is 8 when searching
+ the worktree and 0 otherwise. This option is ignored if git was
+ built without support for POSIX threads.
<tree>...::
Instead of searching tracked files in the working tree, search
diff --git a/builtin/grep.c b/builtin/grep.c
index 0bda900..f698642 100644
--- 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;
--
1.7.8
^ permalink raw reply related
* How to make devs write better commit messages
From: Joseph Huttner @ 2011-12-06 22:55 UTC (permalink / raw)
To: git
So I know that there is a somewhat standard format of commit messages
in Git, which Linus outlines here:
https://github.com/torvalds/subsurface/blob/master/README#L164
Trouble is, when most people go to commit, the file that the editor
opens has no reminder of how to write a proper commit message. Often
I find myself having to go back through the commit log, or consulting
the above link.
I propose two things:
1. An optional flag in the Git config that, if set, shows the format
of a typical commit message in your commit message template.
2. The ability to modify this commit message template. Many teams
use automated tools to read commit messages and then do automated
tasks based on that data, like comment an RT ticket. Thus, developers
need to be reminded of these team-specific settings as well.
What are your thoughts?
The bottom line is that good commit messages are really important, so
we should make it as easy as possible for developers to go ahead and
write a perfect commit message every time they commit code.
E.g.
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit
# On branch master
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# modified: application/views/layouts/layout.phtml
#
# Untracked files:
# (use "git add <file>..." to include in what will be committed)
#
# public/js/databases/
#
# How to properly format your commit message:
#
# Header line: explaining the commit in one line
#
# Body of commit message is a few lines of text, explaining things
# in more detail, possibly giving some background about the issue
# being fixed, etc etc.
#
# The body of the commit message can be several paragraphs, and
# please do proper word-wrap and keep columns shorter than about
# 74 characters or so. That way "git log" will show things
# nicely even when it's indented.
#
# RT: 123, 456 [a comma-separated list of RT tickets this commit refers to]
#
^ 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