* [PATCH 4/4] test: commit --amend should honor --no-edit
From: Jonathan Nieder @ 2011-12-07 14:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <20111207144217.GA30157@elie.hsd1.il.comcast.net>
A quick test to make sure git doesn't lose the functionality added by
the recent patch "commit: honor --no-edit", plus another test to check
the classical --edit use case (use with "-m").
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7501-commit.sh | 40 ++++++++++++++++++++++++++++++++++++++++
1 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index bf025df6..c462bf3b 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -94,6 +94,46 @@ test_expect_success 'amend commit' '
EDITOR=./editor git commit --amend
'
+test_expect_success 'set up editor' '
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ sed -e "s/unamended/amended/g" <"$1" >"$1-"
+ mv "$1-" "$1"
+ EOF
+ chmod 755 editor
+'
+
+test_expect_success 'amend without launching editor' '
+ echo unamended >expect &&
+ git commit --allow-empty -m "unamended" &&
+ echo needs more bongo >file &&
+ git add file &&
+ EDITOR=./editor git commit --no-edit --amend &&
+ git diff --exit-code HEAD -- file &&
+ git diff-tree -s --format=%s HEAD >msg &&
+ test_cmp expect msg
+'
+
+test_expect_success '--amend --edit' '
+ echo amended >expect &&
+ git commit --allow-empty -m "unamended" &&
+ echo bongo again >file &&
+ git add file &&
+ EDITOR=./editor git commit --edit --amend &&
+ git diff-tree -s --format=%s HEAD >msg &&
+ test_cmp expect msg
+'
+
+test_expect_success '-m --edit' '
+ echo amended >expect &&
+ git commit --allow-empty -m buffer &&
+ echo bongo bongo >file &&
+ git add file &&
+ EDITOR=./editor git commit -m unamended --edit &&
+ git diff-tree -s --format=%s HEAD >msg &&
+ test_cmp expect msg
+'
+
test_expect_success '-m and -F do not mix' '
echo enough with the bongos >file &&
test_must_fail git commit -F msg -m amending .
--
1.7.8.rc3
^ permalink raw reply related
* [PATCH 3/4] t7501 (commit): modernize style
From: Jonathan Nieder @ 2011-12-07 14:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <20111207144217.GA30157@elie.hsd1.il.comcast.net>
Put the opening quote starting each test on the same line as the
test_expect_* invocation. While at it:
- guard commands that prepare test input for individual tests in
the same test_expect_success, so their scope is clearer and
errors at that stage can be caught;
- use the compare_diff_patch helper function when comparing patches;
- use single-quotes in preference to double-quotes and <<\EOF in
preference to <<EOF, to save readers the trouble of looking for
variable interpolations;
- lift the setting of the $author variable used throughout the
test script to the top of the test script;
- include "setup" in the titles of test assertions that prepare for
later ones to make it more obvious which tests can be skipped;
- use test_must_fail instead of "if ...; then:; else false; fi",
for clarity and to catch segfaults when they happen;
- break up some pipelines into separate commands that read and write
to ordinary files, and test the exit status at each stage;
- chain commands with &&. Breaks in a test assertion's && chain can
potentially hide failures from earlier commands in the chain;
- combine two initial tests that do not make as much sense alone.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Miscellaneous cleanups.
t/t7501-commit.sh | 278 ++++++++++++++++++++++++++---------------------------
1 files changed, 136 insertions(+), 142 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 9c507b08..bf025df6 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -8,39 +8,39 @@
test_description='git commit'
. ./test-lib.sh
+. "$TEST_DIRECTORY/diff-lib.sh"
+
+author='The Real Author <someguy@his.email.org>'
test_tick
-test_expect_success \
- "initial status" \
- "echo 'bongo bongo' >file &&
- git add file"
-
-test_expect_success "Constructing initial commit" '
+test_expect_success 'initial status' '
+ echo bongo bongo >file &&
+ git add file &&
git status >actual &&
test_i18ngrep "Initial commit" actual
'
-test_expect_success \
- "fail initial amend" \
- "test_must_fail git commit --amend"
+test_expect_success 'fail initial amend' '
+ test_must_fail git commit --amend
+'
-test_expect_success \
- "initial commit" \
- "git commit -m initial"
+test_expect_success 'setup: initial commit' '
+ git commit -m initial
+'
-test_expect_success \
- "invalid options 1" \
- "test_must_fail git commit -m foo -m bar -F file"
+test_expect_success '-m and -F do not mix' '
+ test_must_fail git commit -m foo -m bar -F file
+'
-test_expect_success \
- "invalid options 2" \
- "test_must_fail git commit -C HEAD -m illegal"
+test_expect_success '-m and -C do not mix' '
+ test_must_fail git commit -C HEAD -m illegal
+'
-test_expect_success \
- "using paths with -a" \
- "echo King of the bongo >file &&
- test_must_fail git commit -m foo -a file"
+test_expect_success 'paths and -a do not mix' '
+ echo King of the bongo >file &&
+ test_must_fail git commit -m foo -a file
+'
test_expect_success PERL 'can use paths with --interactive' '
echo bong-o-bong >file &&
@@ -50,120 +50,123 @@ test_expect_success PERL 'can use paths with --interactive' '
git reset --hard HEAD^
'
-test_expect_success \
- "using invalid commit with -C" \
- "test_must_fail git commit -C bogus"
-
-test_expect_success \
- "testing nothing to commit" \
- "test_must_fail git commit -m initial"
-
-test_expect_success \
- "next commit" \
- "echo 'bongo bongo bongo' >file && \
- git commit -m next -a"
-
-test_expect_success \
- "commit message from non-existing file" \
- "echo 'more bongo: bongo bongo bongo bongo' >file && \
- test_must_fail git commit -F gah -a"
-
-# Empty except stray tabs and spaces on a few lines.
-sed -e 's/@$//' >msg <<EOF
- @
-
- @
-Signed-off-by: hula
-EOF
-test_expect_success \
- "empty commit message" \
- "test_must_fail git commit -F msg -a"
-
-test_expect_success \
- "commit message from file" \
- "echo 'this is the commit message, coming from a file' >msg && \
- git commit -F msg -a"
-
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/a file/an amend commit/g" < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
-
-test_expect_success \
- "amend commit" \
- "EDITOR=./editor git commit --amend"
-
-test_expect_success \
- "passing -m and -F" \
- "echo 'enough with the bongos' >file && \
- test_must_fail git commit -F msg -m amending ."
-
-test_expect_success \
- "using message from other commit" \
- "git commit -C HEAD^ ."
-
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/amend/older/g" < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
-
-test_expect_success \
- "editing message from other commit" \
- "echo 'hula hula' >file && \
- EDITOR=./editor git commit -c HEAD^ -a"
-
-test_expect_success \
- "message from stdin" \
- "echo 'silly new contents' >file && \
- echo commit message from stdin | git commit -F - -a"
-
-test_expect_success \
- "overriding author from command line" \
- "echo 'gak' >file && \
- git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a >output 2>&1"
-
-test_expect_success \
- "commit --author output mentions author" \
- "grep Rubber.Duck output"
-
-test_expect_success PERL \
- "interactive add" \
- "echo 7 | git commit --interactive | grep 'What now'"
-
-test_expect_success PERL \
- "commit --interactive doesn't change index if editor aborts" \
- "echo zoo >file &&
+test_expect_success 'using invalid commit with -C' '
+ test_must_fail git commit -C bogus
+'
+
+test_expect_success 'nothing to commit' '
+ test_must_fail git commit -m initial
+'
+
+test_expect_success 'setup: non-initial commit' '
+ echo bongo bongo bongo >file &&
+ git commit -m next -a
+'
+
+test_expect_success 'commit message from non-existing file' '
+ echo more bongo: bongo bongo bongo bongo >file &&
+ test_must_fail git commit -F gah -a
+'
+
+test_expect_success 'empty commit message' '
+ # Empty except stray tabs and spaces on a few lines.
+ sed -e "s/@//g" >msg <<-\EOF &&
+ @ @
+ @@
+ @ @
+ @Signed-off-by: hula@
+ EOF
+ test_must_fail git commit -F msg -a
+'
+
+test_expect_success 'setup: commit message from file' '
+ echo this is the commit message, coming from a file >msg &&
+ git commit -F msg -a
+'
+
+test_expect_success 'amend commit' '
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ sed -e "s/a file/an amend commit/g" < "$1" > "$1-"
+ mv "$1-" "$1"
+ EOF
+ chmod 755 editor &&
+ EDITOR=./editor git commit --amend
+'
+
+test_expect_success '-m and -F do not mix' '
+ echo enough with the bongos >file &&
+ test_must_fail git commit -F msg -m amending .
+'
+
+test_expect_success 'using message from other commit' '
+ git commit -C HEAD^ .
+'
+
+test_expect_success 'editing message from other commit' '
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ sed -e "s/amend/older/g" < "$1" > "$1-"
+ mv "$1-" "$1"
+ EOF
+ chmod 755 editor &&
+ echo hula hula >file &&
+ EDITOR=./editor git commit -c HEAD^ -a
+'
+
+test_expect_success 'message from stdin' '
+ echo silly new contents >file &&
+ echo commit message from stdin |
+ git commit -F - -a
+'
+
+test_expect_success 'overriding author from command line' '
+ echo gak >file &&
+ git commit -m author \
+ --author "Rubber Duck <rduck@convoy.org>" -a >output 2>&1 &&
+ grep Rubber.Duck output
+'
+
+test_expect_success PERL 'interactive add' '
+ echo 7 |
+ git commit --interactive |
+ grep "What now"
+'
+
+test_expect_success PERL "commit --interactive doesn't change index if editor aborts" '
+ echo zoo >file &&
test_must_fail git diff --exit-code >diff1 &&
- (echo u ; echo '*' ; echo q) |
- (EDITOR=: && export EDITOR &&
- test_must_fail git commit --interactive) &&
+ (echo u ; echo "*" ; echo q) |
+ (
+ EDITOR=: &&
+ export EDITOR &&
+ test_must_fail git commit --interactive
+ ) &&
git diff >diff2 &&
- test_cmp diff1 diff2"
+ compare_diff_patch diff1 diff2
+'
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/good/bad/g" < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
+test_expect_success 'editor not invoked if -F is given' '
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ sed -e s/good/bad/g <"$1" >"$1-"
+ mv "$1-" "$1"
+ EOF
+ chmod 755 editor &&
-cat >msg <<EOF
-A good commit message.
-EOF
+ echo A good commit message. >msg &&
+ echo moo >file &&
-test_expect_success \
- 'editor not invoked if -F is given' '
- echo "moo" >file &&
- EDITOR=./editor git commit -a -F msg &&
- git show -s --pretty=format:"%s" | grep -q good &&
- echo "quack" >file &&
- echo "Another good message." | EDITOR=./editor git commit -a -F - &&
- git show -s --pretty=format:"%s" | grep -q good
- '
+ EDITOR=./editor git commit -a -F msg &&
+ git show -s --pretty=format:%s >subject &&
+ grep -q good subject &&
+
+ echo quack >file &&
+ echo Another good message. |
+ EDITOR=./editor git commit -a -F - &&
+ git show -s --pretty=format:%s >subject &&
+ grep -q good subject
+'
test_expect_success 'partial commit that involves removal (1)' '
@@ -197,7 +200,6 @@ test_expect_success 'partial commit that involves removal (3)' '
'
-author="The Real Author <someguy@his.email.org>"
test_expect_success 'amend commit to fix author' '
oldtick=$GIT_AUTHOR_DATE &&
@@ -326,7 +328,6 @@ test_expect_success 'multiple -m' '
'
-author="The Real Author <someguy@his.email.org>"
test_expect_success 'amend commit to fix author' '
oldtick=$GIT_AUTHOR_DATE &&
@@ -353,15 +354,8 @@ test_expect_success 'git commit <file> with dirty index' '
test_expect_success 'same tree (single parent)' '
- git reset --hard
-
- if git commit -m empty
- then
- echo oops -- should have complained
- false
- else
- : happy
- fi
+ git reset --hard &&
+ test_must_fail git commit -m empty
'
--
1.7.8.rc3
^ permalink raw reply related
* [PATCH 2/4] test: remove a porcelain test that hard-codes commit names
From: Jonathan Nieder @ 2011-12-07 14:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <20111207144217.GA30157@elie.hsd1.il.comcast.net>
The rev-list output in this test depends on the details of test_tick's
dummy dates and the choice of hash function. Worse, it depends on the
order and nature of commits made in the earlier tests, so adding new
tests or rearranging existing ones breaks it.
It would be nice to check that "git commit" and commit-tree name
objects consistently and that commit objects' text is as documented,
but this particular test checks everything at once and hence is not a
robust test for that. Remove it.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hence the odd hunk in the previous patch.
Maybe this doesn't belong in this series, but it's the kind of thing
that drives me batty when writing new tests.
t/t7501-commit.sh | 20 --------------------
1 files changed, 0 insertions(+), 20 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index da75abc1..9c507b08 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -144,10 +144,6 @@ test_expect_success PERL \
git diff >diff2 &&
test_cmp diff1 diff2"
-test_expect_success \
- "showing committed revisions" \
- "git rev-list HEAD >current"
-
cat >editor <<\EOF
#!/bin/sh
sed -e "s/good/bad/g" < "$1" > "$1-"
@@ -168,22 +164,6 @@ test_expect_success \
echo "Another good message." | EDITOR=./editor git commit -a -F - &&
git show -s --pretty=format:"%s" | grep -q good
'
-# We could just check the head sha1, but checking each commit makes it
-# easier to isolate bugs.
-
-cat >expected <<\EOF
-285fcf7ec0d61b14249dfdb4c1e1fe03eaf15ee0
-0b8148b9afce917b87d71199b900466dc8ea8b6e
-43fb8826314939ce79a856face7953557fdca3d1
-eaa04bc3ae0f0b003f7f1d86bf869ec5d73eaf3e
-ee1963b250ee0f02a3fe37be0e4a02bb5af6a1ad
-b49f306003c627361a0304d151a6b4c8b26af6a1
-402702b49136e7587daa9280e91e4bb7cb2179f7
-EOF
-
-test_expect_success \
- 'validate git rev-list output.' \
- 'test_cmp expected current'
test_expect_success 'partial commit that involves removal (1)' '
--
1.7.8.rc3
^ permalink raw reply related
* [PATCH 1/4] test: add missing "&&" after echo command
From: Jonathan Nieder @ 2011-12-07 14:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <20111207144217.GA30157@elie.hsd1.il.comcast.net>
This test wants to modify a file and commit the change, but because of
a missing separator between commands it is parsed as a single "echo"
command.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7501-commit.sh | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3ad04363..da75abc1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -60,7 +60,7 @@ test_expect_success \
test_expect_success \
"next commit" \
- "echo 'bongo bongo bongo' >file \
+ "echo 'bongo bongo bongo' >file && \
git commit -m next -a"
test_expect_success \
@@ -172,11 +172,12 @@ test_expect_success \
# easier to isolate bugs.
cat >expected <<\EOF
-72c0dc9855b0c9dadcbfd5a31cab072e0cb774ca
-9b88fc14ce6b32e3d9ee021531a54f18a5cf38a2
-3536bbb352c3a1ef9a420f5b4242d48578b92aa7
-d381ac431806e53f3dd7ac2f1ae0534f36d738b9
-4fd44095ad6334f3ef72e4c5ec8ddf108174b54a
+285fcf7ec0d61b14249dfdb4c1e1fe03eaf15ee0
+0b8148b9afce917b87d71199b900466dc8ea8b6e
+43fb8826314939ce79a856face7953557fdca3d1
+eaa04bc3ae0f0b003f7f1d86bf869ec5d73eaf3e
+ee1963b250ee0f02a3fe37be0e4a02bb5af6a1ad
+b49f306003c627361a0304d151a6b4c8b26af6a1
402702b49136e7587daa9280e91e4bb7cb2179f7
EOF
--
1.7.8.rc3
^ permalink raw reply related
* Re: Undo a commit that is already pushed to central server and merged to several branches
From: Ramkumar Ramachandra @ 2011-12-07 14:42 UTC (permalink / raw)
To: Matthias Fechner; +Cc: Git Mailing List
In-Reply-To: <4EDF74EC.6090504@fechner.net>
Hi Matthias,
Matthias Fechner wrote:
> [...]
> I continued to work then on different branches and merged the bad master
> branch to all my other branches.
> [...]
>
> What I would like to do is move this bogus commit into a different branch
> and remove all changes from this bogus commit from every branch.
If I understand correctly, each of your branches looks like*:
o <--- HEAD of branch; merge commit referencing bogus commit
| \
o \ <--- This is where you want to move the HEAD to
| \
o o <-- Bogus commit from master branch
|
o
|
o <-- Branch born
Assuming that you actually want to rewrite the history, the situation
calls for a git-reset(1). Just "git reset --hard HEAD~1" on each of
your branches (Caution: first understand what it does!) and you'll
rewind the HEAD to "undo" the bad merge. After that you can just "git
push +foo:foo" to overwrite the foo branch on your server. If you
don't want to rewrite anything and simply commit the inverse of the
bad commit, see git-revert(1).
* If you're having difficulty understanding the diagram, please read:
http://eagain.net/articles/git-for-computer-scientists/
Cheers.
-- Ram
^ permalink raw reply
* [PATCH/RFC 0/4] Re: commit: honor --no-edit
From: Jonathan Nieder @ 2011-12-07 14:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <7vk469fm9j.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> And this should fix it (only lightly tested).
>
> -- >8 --
> Subject: [PATCH] commit: honor --no-edit
>
> After making fixes to the contents to be committed, it is not unusual to
> update the current commit without rewording the message. Idioms to do
> tell "commit --amend" that we do not need an editor have been:
>
> $ EDITOR=: git commit --amend
> $ git commit --amend -C HEAD
>
> but that was only because a more natural
>
> $ git commit --amend --no-edit
>
> did not honour "--no-edit" option.
I like it.
Here are a couple of tests. The three patches before are just to make
it less frightening to add to the relevant test script.
Jonathan Nieder (4):
test: add missing "&&" after echo command
test: remove a test of porcelain that hardcodes commit ids
t7501 (commit): modernize style
test: commit --amend should honor --no-edit
t/t7501-commit.sh | 335 ++++++++++++++++++++++++++++-------------------------
1 files changed, 175 insertions(+), 160 deletions(-)
^ permalink raw reply
* Undo a commit that is already pushed to central server and merged to several branches
From: Matthias Fechner @ 2011-12-07 14:15 UTC (permalink / raw)
To: Git Mailing List
Dear git list,
I commited two files into the master branch (later I figured out that
the files included a lot bugs).
I continued to work then on different branches and merged the bad master
branch to all my other branches.
I pushed the master branch then to the server including the bogus commit.
What I would like to do is move this bogus commit into a different
branch and remove all changes from this bogus commit from every branch.
Is this possible and how?
Bye
Matthias
--
"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs, and the universe trying to
produce bigger and better idiots. So far, the universe is winning." --
Rich Cook
^ permalink raw reply
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Stefan Näwe @ 2011-12-07 10:26 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
Am 06.12.2011 21:32, schrieb Pat Thoyts:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Good News!
Thanks to all involved!
Stefan
--
----------------------------------------------------------------
/dev/random says: Do steam rollers really roll steam?
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
^ permalink raw reply
* &&-chaining tester
From: Jonathan Nieder @ 2011-12-07 10:08 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, git
In-Reply-To: <CALkWK0nNtvrLHxQv17jfrFQ=BcwLfgh7eE9X-nDCCYY0nsOskg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Missing the chaining '&&' seems to be quite a common
> error: I vaguely remember seeing a patch to detect this sometime ago.
> Jonathan?
I guess you mean [1].
If you want to deploy it, you have three choices:
- maintain a list of tests known to have broken &&-chaining, and
set GIT_SKIP_TESTS to avoid them
- fix all cases of broken &&-chaining. For extra bonus points,
send out those patches, respond to reviews, and gently usher
them into Junio's tree
- modify the &&-chaining tester to output a list of tests encountered
with broken &&-chaining instead of halting on the first one.
Some day, I'd like to (help) carry out option (b), but please don't
hold your breath.
[1] http://thread.gmane.org/gmane.comp.version-control.git/157903/focus=158265
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 9:56 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0k+GF9qjwYd_TzA3o8FqAH6G+cxREZ5r9x8E2G5k-mmmg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> I suppose they were a combination of all three. I'm sorry- I'll try
> to be more careful and communicative in the future.
Hm, that is not exactly what I was going for, either. First, I didn't
mean to harp on anything in the past --- it was a request regarding
this current review. And on the other hand, I am not sure it is a
matter of you being uncareful or uncommunicative, exactly. It's more
that _I_ am doing a poor job of clearly conveying what Junio described
as easier to learn on one's own at [1].
To put it more concretely:
I am going to be stubborn. I will not ack a patch unless I can
imagine myself, years from now, after running into a bug in a related
area, not being confused by it.
[1] http://thread.gmane.org/gmane.comp.version-control.git/186402/focus=186415
^ permalink raw reply
* Re: [RFC PATCH] git-p4: introduce asciidoc documentation
From: Luke Diamand @ 2011-12-07 9:46 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git
In-Reply-To: <20111203235328.GA3866@arf.padd.com>
On 03/12/11 23:53, Pete Wyckoff wrote:
> Add proper documentation for git-p4. Delete the old .txt
> documentation from contrib/fast-import.
> ---
>
> I'd appreciate review by git-p4 people to make sure I captured
> everything from the old documentation, and to catch any errors.
> There's a fair amount of new content in here, describing all
> the options and variables. I left out some obscure commands
> on purpose.
>
> Comments from anyone else would be welcome too. Especially
> in the area of generic asciidoc formatting or style in git
> command pages.
That looks good to me, I can't see anything of note, just one minor nit,
below.
Luke
>
> Thanks,
> -- Pete
>
> Documentation/git-p4.txt | 456 ++++++++++++++++++++++++++++++++++++++++
> contrib/fast-import/git-p4.txt | 289 -------------------------
> 2 files changed, 456 insertions(+), 289 deletions(-)
> create mode 100644 Documentation/git-p4.txt
> delete mode 100644 contrib/fast-import/git-p4.txt
> +
>
> +------------
> +$ cd project
> +$ vi foo.h
I think it works with other editors as well, although I've not tried it
myself. Obviously with reduced functionality :-)
> +git-p4.allowSubmit::
> + By default, any branch can be used as the source for a 'git p4
> + submit' operation. This configuration variable , if set, permits only
Spacing around comma in "variable , if set"
Luke
^ permalink raw reply
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Sebastian Schuberth @ 2011-12-07 9:44 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
On 06.12.2011 21:32, Pat Thoyts wrote:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Thanks a lot, Pat, for making the release!
--
Sebastian Schuberth
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Andreas T.Auer @ 2011-12-07 9:07 UTC (permalink / raw)
To: git
In-Reply-To: <4ED5E9D2.4060503@web.de>
Jens Lehmann wrote:
> Am 30.11.2011 01:55, schrieb Max Krasnyansky:
> I'm working on a patch series to teach Git to optionally update the
> submodules work trees on checkout, reset merge and so on, but I'm not
> there yet.
>
>> I'm thinking about adding a config option that would enable automatic
>> submodule update but wanted to see if there is some fundamental reason
>> why it would not be accepted.
Because there is no good way to do so. It would be fine when you just track
the submodules "read-only", but if you are actually working on submodules,
it is a bad idea to always get a detached HEAD. It is also a bad idea to
merge or rebase on the currently checkedout branch. Because if you are
working on a maint branch in the submodule and then you checkout a pu branch
in the superproject, because you have forgotten that maint branch in the
submodule then all the proposed updates go to the maintenance branch -> bad.
So auto-update is not easy. But below I describe an idea that might solve
these issues and help auto-udpate to work in a sane way.
> I think adding something like an "submodule.autoupdate" config makes lots
> of sense, but IMO it should affect all work tree updating porcelain
> commands, not just merge.
I was thinking about submodule integration and had the idea to bind a
submodule to the superproject by having special references in the submodule
like refs/super/master, refs/super/featureX... So these references are like
tracking branches for the refs/heads/* of the superproject.
If you have tracking branches, the supermodule can just update the
corresponding branch. If this branch is currently checkedout and the work
area is clean, then the work area is updated, too. If there is currently a
local branch or a diffent super-branch checked out then the working area
should be considered "detached" from the superproject and not updated.
With this concept you could even switch branches in the superproject and the
attached submodules follow - still having no detached HEAD. When you want to
do some local work on the submodule you checkout a local branch and merge
back into the super branch later. The head of that super branch might have
changed by the update procedure meanwhile, but that is fine, then you just
have a merge instead of a fast-forward.
Another nice feature would be a recursive commit. So all changed index files
in the _attached_ submodules would first be committed in their submodules
and then the superproject commits too - all with one command. Currently it
feels a little bit like CVS - commit one file(submodule), commit the other
file(submodule) and then apply a label(commit the superproject) to keep the
changes together.
If the submodule is not attached the commit in the superproject can still
detect changes that have been made to the corresponding tracking branch and
pick these up.
As a summary: Tracking submodule branches in the superproject instead of
only the current HEAD of the submodule gives you more freedom to install
sane auto-update procedures. Even though it will raise a lot of detailed
questions like "should the refs/super/* be pushed/pulled when syncing the
submodule repositories".
^ permalink raw reply
* Re: [PATCH 2/2] run-command: Add interpreter permissions check
From: Frans Klaver @ 2011-12-07 8:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk469e2rn.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> If a script is started and the interpreter of that script given in the
>> shebang cannot be started due to permissions, we can get a rather
>> obscure situation. All permission checks pass for the script itself,
>> but we still get EACCES from execvp.
>>
>> Try to find out if the above is the case and warn the user about it.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>> run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
>> t/t0061-run-command.sh | 22 ++++++++++++++++
>> 2 files changed, 82 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 5e38c5a..b8cf8d4 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>> return 0;
>> }
>>
>> +static void check_interpreter(const char *cmd)
>> +{
>> + FILE *f;
>> + struct strbuf sb = STRBUF_INIT;
>> + /* bash reads an 80 character line when determining the interpreter.
>> + * BSD apparently only allows 32 characters, as it is the size of
>> + * your average binary executable header.
>> + */
>> + char firstline[80];
>> + char *interpreter = NULL;
>> + size_t s, i;
>> +
>> + f = fopen(cmd, "r");
>> + if (!f) {
>> + error("cannot open file '%s': %s\n", cmd, strerror(errno));
>> + return;
>> + }
>> +
>> + s = fread(firstline, 1, sizeof(firstline), f);
>> + if (s < 2) {
>> + trace_printf("cannot determine file type");
>> + fclose(f);
>> + return;
>> + }
>> +
>> + if (firstline[0] != '#' || firstline[1] != '!') {
>> + trace_printf("file '%s' is not a script or"
>> + " is a script without '#!'", cmd);
>> + fclose(f);
>> + return;
>> + }
>
> Nice touches to silently pass scripts that do not begin with she-bang.
>
>> +
>> + /* see if the given path has the executable bit set */
>> + for (i = 2; i < s; i++) {
>> + if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
>> + interpreter = firstline + i;
>> +
>> + if (interpreter && (firstline[i] == ' ' ||
>> + firstline[i] == '\n')) {
>
> Curious.
>
> "#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?
Apparently so. Thanks for catching.
>> + strbuf_add(&sb, interpreter,
>> + (firstline + i) - interpreter);
>> + break;
>> + }
>
> Wouldn't strcspn() work better instead of this loop?
Probably. Will revise.
>> + }
>> + if (!sb.len) {
>> + error("could not determine interpreter");
>> + strbuf_release(&sb);
>> + return;
>> + }
>> +
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("bad interpreter: no read/execute permissions on '%s'\n",
>> + sb.buf);
>> +
>> + strbuf_release(&sb);
>> +}
>> +
>> static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> {
>> /* man 2 execve states that EACCES is returned for:
>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> char *next;
>>
>> if (strchr(cmd, '/')) {
>> - if (!have_read_execute_permissions(cmd))
>> - error("no read/execute permissions on '%s'\n", cmd);
>> + if (have_read_execute_permissions(cmd))
>> + check_interpreter(cmd);
>
> I would have expected the overall logic to be more like this:
>
> if we cannot read and execute it then
> that in itself is an error (i.e. the error message from [1/2])
> else if we can read it then
> let's see if there is an error in the interpreter.
>
> It is unnatural to see "if we can read and execute, then see if there is
> anything wrong with the interpreter" and _nothing else_ here. If you made
> the "have_read_execute_permissions()" to issue the error message you used
> to give in your [1/2] patch here, that is OK from the point of view of the
> overall code structure, but then the function is no longer "do we have
> permissions" boolean check and needs to be renamed. And if you didn't,
> then I have to wonder why we do not need the error message you added in
> your [1/2].
Hm, yea makes sense. I'll rethink this a bit.
Again, thanks for the review.
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-07 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
Thanks for the review. There's a lot of things you mention that I
either didn't see (staring blind, you know) or that I didn't know of.
On Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> + struct stat s;
>> + trace_printf("checking '%s'\n", path);
>> +
>> + if (stat(path, &s) < 0) {
>> + ...
>> + /* check world permissions */
>> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> + return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?
Probably. I'll use access instead in a reroll.
> I also think that your permission check is incorrectly implemented.
>
> $ cd /var/tmp && date >j && chmod 044 j && ls -l j
> ----r--r-- 1 junio junio 29 Dec 6 14:32 j
> $ cat j
> cat: j: Permission denied
> $ su pogo
> Password:
> $ cat j
> Tue Dec 6 14:32:23 PST 2011
>
> That's a world-readable but unreadable-only-to-me file.
Hmm, this is a case that didn't fit my expectations. Thanks for catching.
>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> +{
>> + /* man 2 execve states that EACCES is returned for:
>
> /*
> * Just a style, but we tend to write multi-line comment like
> * this, without anything else on opening and closing lines of
> * the comment block.
> */
>
>> + * - The file system is mounted noexec
>> + */
>> + struct strbuf sb = STRBUF_INIT;
>> + char *path = getenv("PATH");
>> + char *next;
>> +
>> + if (strchr(cmd, '/')) {
>> + if (!have_read_execute_permissions(cmd))
>> + error("no read/execute permissions on '%s'\n", cmd);
>> + return;
>> + }
>
> Ok, execvp() failed and "cmd" has at least one slash, so we know we did
> not look for it in $PATH. We check only one and return (did you need
> getenv() in that case?).
Obviously not. Missed that.
>
>> + for (;;) {
>> + next = strchrnul(path, ':');
>> + if (path < next)
>> + strbuf_add(&sb, path, next - path);
>> + else
>> + strbuf_addch(&sb, '.');
>
> Nice touch that you did not forget an empty component on $PATH.
Yes, that's a relic from me starting work based on one of your
proposed patches[1]. So that one goes to you.
[1] http://article.gmane.org/gmane.comp.version-control.git/171838
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n", sb.buf);
>
> Don't you want to continue here upon error, after resetting sb? You just
> saw the directory is unreadble, so you know next file_exists() will fail
> before you try it.
Yes. I thought about that. I didn't do that because of the fact that I
had to do more than just resetting sb. The path variable has to be
updated as well. I had the choice of adding a level of indentation {},
duplicating the code, or just do a check I know before will fail.
There's probably something to say for each one of them. I'll probably
refactor that a bit more.
>> + if (sb.len && sb.buf[sb.len - 1] != '/')
>> + strbuf_addch(&sb, '/');
>> + strbuf_addstr(&sb, cmd);
>> +
>> + if (file_exists(sb.buf)) {
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n",
>> + sb.buf);
>> + else
>> + warn("file '%s' exists and permissions "
>> + "seem OK.\nIf this is a script, see if you "
>> + "have sufficient privileges to run the "
>> + "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?
I don't know/remember. It seemed like a natural thing to do, but I'll find out.
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
^in
> time is well spent.
Sorry for the nonsense. :) And now that I look back over previous
revisions of the patches, I see that I _hadn't_ clearly complained
about the same aspects of these particular commit messages before.
So what am I talking about here?
I guess it is just a pattern: commit messages I have looked at in the
sequencer series lately seem to focus too much on implementation
details and not enough on the "big picture" of what the user or
internal API user will experience. One symptom is that I get lost in
reading the commit message without looking at the patch. Another
symptom is that the commit messages tend to mention the particular
(private) functions that were changed, instead of the more prominent
(often public) callers that the reader might have cause to call.
Hoping that clarifies a little,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Ramkumar Ramachandra @ 2011-12-07 8:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Hi Jonathan,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this. I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
> - you didn't notice them
> - they were wrong
> - you noticed them and they were describing real problems, but it
> wasn't worth the time to fix them
>
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.
I suppose they were a combination of all three. I'm sorry- I'll try
to be more careful and communicative in the future.
Thanks for this round of reviews!
-- Ram
^ permalink raw reply
* Re: Re: Git Submodule Problem - Bug?
From: Manuel Koller @ 2011-12-07 8:21 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Jens Lehmann, Fredrik Gustafsson, Thomas Rast, git
In-Reply-To: <20111129220303.GA2812@sandbox-rc.fritz.box>
> How about this:
>
> The user issues 'git submodule add foo' and we discover that there is
> already a local clone under the name foo. Git then asks something like
> this
>
> Error when adding: There is already a local submodule under the
> name 'foo'.
>
> You can either rename the submodule to be added to a different
> name or manually remove the local clone underneath
> .git/modules/foo. If you want to remove the local clone please
> quit now.
>
> We strongly suggest that you give each submodule a unique name.
> Note: This name is independent from the path it is bound to.
>
> What do you want me to do ([r]ename it, [Q]uit) ?
>
> When the user chooses 'rename' git will prompt for a new name.
>
> If we are going to support the remove use case with add we additionally
> need some logic to deal with it during update (which is not supported
> yet AFAIK). But we probably need this support anyway since between
> removal and adding a new submodule under the same can be a long time.
> If users switch between such ancient history and the new history we
> would have the same conflict.
>
> We could of course just error out and tell the user that he has to give
> the submodule an uniqe name. If the user does not do so leave it to him
> to deal with the situation manually.
>
> What do you think?
>
> Cheers Heiko
Prompt to choose another name would be fine I guess - but it solves
the problem only if the submodule has been initialized already. There
could be a submodule of the same name in another branch, which I
haven't checked out yet, for example. The user would have to be forced
choose a unique name for every submodule.
Anyway, it seems impossible to handle a name clash automatically,
since there are good reasons to have different urls for the same
submodule. Having read the thread linked by Junio, the only way out
seems to be a kind of url rewrite scheme and using the url as name.
Doesn't it solve all the problems?
- the url is more or less unique (there are problems now if we have to
different submodules at the same path, which is much more likely to
happen than a different repository at the same url some time in the
future)
- after a change of the submodule's url, we can still check out old
commits in a comfortable way
- we could have the same submodule at different paths, but downloaded only once
- the user is not forced to do anything, but the .gitmodule config can
still be overruled if necessary
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:17 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
> ($gname/186365) implemented.
Thanks for this. I am worried that some of my comments in previous
reviews (especially about change descriptions) were not addressed,
which could mean one of a few things:
- you didn't notice them
- they were wrong
- you noticed them and they were describing real problems, but it
wasn't worth the time to fix them
Fine. But I would like to know which case they fell into, so I can
learn in order to do a better job reviewing the future and know my
time is well spent.
The series makes various changes, all essentially good, which leaves
me all the more motivated to figure out how to get this sort of thing
in smoothly without making life difficult for people in the future
tracking down regressions.
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: Thomas Rast @ 2011-12-07 8:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9ED1.8010502@lsrfire.ath.cx>
René Scharfe wrote:
> 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 when searching
> + the worktree and 0 otherwise. This option is ignored if git was
> + built without support for POSIX threads.
[...]
> - nr_threads = (online_cpus() > 1) ? THREADS : 0;
> + nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
It would be more consistent to stick to the pack.threads convention
where 0 means "all of my cores", so to disable threading the user
would set the number of threads to 1. Or were you trying to measure
the contention between the worker thread and the add_work() thread?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: Thomas Rast @ 2011-12-07 8:11 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
René Scharfe wrote:
> Am 02.12.2011 17:15, schrieb René Scharfe:
> > How about adding a parameter to control the number of threads
> > (--threads?) instead that defaults to eight (or five) for the worktree
> > and one for the rest? That would also make benchmarking easier.
>
> Like this:
>
> -- >8 --
> Subject: grep: add parameter --threads
>
> Allow the number of threads to be specified by the user. This makes
> benchmarking the performance impact of different numbers of threads
> much easier.
Sounds good, though in the end we would also want to have a config
variable for the poor OS X users who have to tune their threads
*down*... :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 5/5] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-07 8:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> 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.
Fine. I guess it can be said more clearly while emphasizing public
interfaces over implementation details:
Since the "multiple cherry-pick" feature was introduced (commit
7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track
of the set of commits to be cherry-picked or reverted using
commit_argc and commit_argv variables storing the relevant
command-line parameters.
Future callers as other commands are built in (am, rebase,
sequencer) may find it easier to pass rev-list options to this
machinery in already-parsed form, though. So teach
cmd_cherry_pick and cmd_revert to parse the rev-list arguments
in advance and pass the commit set to pick_revisions() as a
value of type "struct rev_info".
[...]
> @@ -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);
> }
Looks good.
Tests? What happens if I try "git cherry-pick --all"? How about "git
cherry-pick HEAD..HEAD"?
FWIW, with the commit message clarified and with or without new tests,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Thomas Rast @ 2011-12-07 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vfwgxflkv.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > Actually (sadly) I'll have to revise it. It doesn't match much of C++
> > either, and I haven't yet come up with a reasonable regex that
> > matches, say,
> >
> > foo::Bar<int>::t& Baz::operator<<(
> >
> > which I would call ludicrous, but it's valid C++.
>
> Heh, I'd rather not see us go that route, which would either end up
> implementing a C++ parser or reverting the heuristics back to "non-blank
> at the beginning of the line" that was already reasonably useful.
Well, there are many things that we deliberately do not match right
now and for which that's a good thing:
label:
public:
void declaration_only(...);
int global_variable;
At some point I was wondering whether it would be better to just
declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and
otherwise match all '^[A-Za-z].*\(' but I may be missing something.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 5/5] reset: update cache-tree data when appropriate
From: Thomas Rast @ 2011-12-07 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Carlos Martín Nieto
In-Reply-To: <7v62hte1k7.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > @@ -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.
Oh, I just didn't bother with timings because it's the same strategy
that read-tree follows, so I assumed if it was worth it back then, it
should again be a win.
For linux-2.6 as usual and best-of-10:
git reset before: 0:00.25real 0.12user 0.11system
after: 0:00.28real 0.14user 0.12system
git reset --hard before: 0:00.21real 0.13user 0.06system
after: 0:00.18real 0.10user 0.07system
So we spend ~30ms of extra user time, at a possible gain of 30% in
future git-commit invocations, as mentioned in the cover letter. I
think that's worth it :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-12-07 7:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> 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.
Good idea.
> 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.
I don't know what this part is about...
[...]
> --- 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 };
Removing the enum, to expose it in sequencer.h. What does this have
to do with the stated goal of the patch?
> 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";
[... lots of similar changes snipped ...]
> @@ -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)
Ok, now we're at the title feature.
Passing "action" as a separate parameter in addition to "opts" makes
sense since it can vary from item to item on the todo list. It
unfortunately makes reviewers' lives hard since it is easy to miss
additional unconverted uses of opts->action in the function and
compilers will not warn about that.
[...]
> if (index_differs_from("HEAD", 0))
> return error_dirty_index(opts);
[...]
> if (parent && parse_commit(parent) < 0)
> /* TRANSLATORS: The first %s will be "revert" or
> "cherry-pick", the second %s a SHA1 */
> return error(_("%s: cannot parse parent commit %s"),
> action_name(opts), sha1_to_hex(parent->object.sha1));
[...]
> res = do_recursive_merge(base, next, base_label, next_label,
> head, &msgbuf, opts);
Makes sense. This is the command name passed on the command line, not
the action name from the todo list, so it is left alone.
[...]
> @@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
[...]
> - error(opts->action == REVERT
> + error(action == REPLAY_REVERT
> ? _("could not revert %s... %s")
> : _("could not apply %s... %s"),
... and that's the last use of "action" in this function on the
current master branch. Good.
[... more distracting s/REVERT/REPLAY_REVERT/ stuff snipped...]
> @@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
[...]
> -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)
Looks good.
[...]
> -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)
[...]
> - } 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);
> + }
A new error message when an insn line does not start with "pick" or
"revert". Looks like a good change, but what does it have to do with
the subject of the patch?
[...]
> @@ -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;
[...]
> if (status < 0)
> - return NULL;
> + return error(_("Line %d: Malformed object name: %s"), lineno, bol);
(Not about this patch, but an earlier one) What is this idiom about?
Why not
if (get_sha1(bol, commit_sha1))
return error(_(...));
?
[...]
> -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)
Nice to see.
> {
> - struct commit_list **next = todo_list;
> - struct commit *commit;
> + struct replay_insn_list **next = todo_list;
> + struct replay_insn_list item = {0, NULL, NULL};
Style: elsewhere in this file, initializers look like "{ 0, NULL, NULL }"
(with space between the braces and values).
[...]
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
Thanks for writing these.
> 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 &&
Stops in "picked", asking user to resolve conflicts.
> + echo "c" >foo &&
> + git add foo &&
> + git commit &&
User resolves conflicts, as in the previous test.
> + git revert --continue &&
And asks to 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
I wonder if there is some simpler way to check the result of resuming
a cherry-pick sequence, though that doesn't have much to do with the
patch. I guess I'd find
echo d >expect &&
... &&
# index is clean
git update-index --refresh &&
git rev-list HEAD >commits &&
test_line_count = 4 commits &&
test_cmp expect foo &&
test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
a more convincing demonstration that the cherry-pick finished without
incident.
> +'
> +
> +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 &&
Same setup.
> + oldsha=`git rev-parse --short HEAD~1` &&
> + echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
Adding to the insn sheet. Makes sense.
Summing up: I quite like the idea of this patch, but I would be a lot
happier if changes unrelated to its goal were split off to be
considered separately.
> Acked-by: Jonathan Nieder <jrnider@gmail.com>
No, not acked.
Thanks,
Jonathan
^ 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