* Re: [PATCH 2/7] contrib/subtree: Use %B for Split Subject/Body
From: Junio C Hamano @ 2013-01-09 0:41 UTC (permalink / raw)
To: 郑文辉(Techlive Zheng); +Cc: David A. Greene, git
In-Reply-To: <CAPYzjrQ1ngfOwBuzq+Da1Ynd18Vwt8=LCyu2yhE6dX8vivwReg@mail.gmail.com>
"郑文辉(Techlive Zheng)" <techlivezheng@gmail.com> writes:
>> +test_expect_success 'check hash of split' '
>> + spl1=$(git subtree split --prefix subdir) &&
>> + undo &&
>> + git subtree split --prefix subdir --branch splitbr1test &&
>> + check_equal ''"$(git rev-parse splitbr1test)"'' "$spl1"
>> + git checkout splitbr1test &&
>> + new_hash=$(git rev-parse HEAD~2) &&
>> + git checkout mainline &&
>> + check_equal ''"$new_hash"'' "$subdir_hash"
>> +'
>> +
> This test is not test the correct thing, I am currently working on it.
Will keep the topic branch out of 'next' for now.
David, how would you like to handle a reroll of this piece?
^ permalink raw reply
* What's cooking (interim report)
From: Junio C Hamano @ 2013-01-09 0:53 UTC (permalink / raw)
To: git
I'll do the next issue of "What's cooking" after tomorrow's
integration cycle, but here are the highlights.
The following topics that have already graduated to the 'master'
branch have been merged to the 'maint' branch (see the last "What's
cooking" for details of individual topics):
ms/subtree-fixlets
ss/nedmalloc-compilation
jc/maint-fnmatch-old-style-definition
jc/test-portability
jc/maint-fbsd-sh-ifs-workaround
jc/mkstemp-more-careful-error-reporting
jc/test-cvs-no-init-in-existing-dir
jc/maint-test-portability
In addition, the following two patches have been directly applied to
the 'maint' branch:
t1402: work around shell quoting issue on NetBSD
remote-hg: Fix biridectionality -> bidirectionality typos
We will have other bugfix topics merged to 'maint' and hopefully can
tag v1.8.1.1 sometime next week.
The following topics that have been cooking on 'next' have been
merged to the 'master' branch:
kb/maint-bundle-doc
as/test-name-alias-uniquely
ta/remove-stale-translated-tut
tb/test-t9810-no-sed-i
tb/test-t9020-no-which
jk/maint-fast-import-doc-dedup-done
jk/pathspec-literal
Most of these will later be merged to 'maint'.
These topics have been merged to the 'next' branch:
rs/zip-with-uncompressed-size-in-the-header
rs/zip-tests
jn/xml-depends-on-asciidoc-conf
jc/comment-cygwin-win32api-in-makefile
as/api-allocation-doc
jk/unify-exit-code-by-receiving-signal
rs/leave-base-name-in-name-field-of-tar
jl/interrupt-clone-remove-separate-git-dir
jc/merge-blobs
mo/cvs-server-updates
as/dir-c-cleanup
jk/config-uname
Also several new topics are parked in 'pu' and I think they are all
ready for 'next'.
Thanks.
^ permalink raw reply
* Re: [PATCH] add warning for depth=0 in git clone.
From: Duy Nguyen @ 2013-01-09 0:57 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Jonathan Nieder, schlotter, Ralf.Wildenhues, git
In-Reply-To: <1357632422-5686-1-git-send-email-stefanbeller@googlemail.com>
On Tue, Jan 8, 2013 at 3:07 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> ---
> builtin/clone.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ec2f75b..5e91c1e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -818,6 +818,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> remote = remote_get(option_origin);
> transport = transport_get(remote, remote->url[0]);
>
> + if (option_depth && transport->smart_options->depth < 1)
> + die(_("--depth less or equal 0 makes no sense; read manpage."));
> +
Isn't this too early for the check? The following code is
> if (!is_local) {
> if (!transport->get_refs_list || !transport->fetch)
> die(_("Don't know how to clone %s"), transport->url);
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
where transport_set_option() calls set_git_option() to initialize
transport->smart_options->depth. A check should be done after this
point.
--
Duy
^ permalink raw reply
* Re: [PATCH] t1402: work around shell quoting issue on NetBSD
From: Greg Troxel @ 2013-01-09 1:27 UTC (permalink / raw)
To: René Scharfe
Cc: git discussion list, Junio C Hamano, Jonathan Nieder,
Michael Haggerty
In-Reply-To: <50EC8025.8000707@lsrfire.ath.cx>
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> invalid_ref() constructs a test case description using its last argument,
> but the shell seems to split it up into two pieces if it contains a
> space. Minimal test case:
This is indeed a bug in NetBSD's shell, which I reported after finding
this test case problem, and I think someone is working on a fix. But
because git does not intend to be a shell torture test, if it's possible
to avoid bugs in a reasonable way, I think it's nice to do so.
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply
* Re: [PATCH] t1402: work around shell quoting issue on NetBSD
From: Greg Troxel @ 2013-01-09 2:07 UTC (permalink / raw)
To: René Scharfe; +Cc: git discussion list
In-Reply-To: <50EC8025.8000707@lsrfire.ath.cx>
[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> The test fails for me on NetBSD 6.0.1 and reports:
>
> ok 1 - ref name '' is invalid
> ok 2 - ref name '/' is invalid
> ok 3 - ref name '/' is invalid with options --allow-onelevel
> ok 4 - ref name '/' is invalid with options --normalize
> error: bug in the test script: not 2 or 3 parameters to test-expect-success
>
> The alleged bug is in this line:
>
> invalid_ref NOT_MINGW '/' '--allow-onelevel --normalize'
The bug in NetBSD's sh has been fixed in -current:
http://gnats.netbsd.org/47361
and the change will almost certainly make it to the -6 and -5 release
branches.
With the fixed sh:
414c78c (with the workaround): t1402 passes
69637e5 (without the workaround): t1402 passes
With the buggy sh,
414c78c (with the workaround): t1402 passes
69637e5 (without workaround): t1402 fails
so I can confirm that the workaround is successful on NetBSD 5.
Thanks for addressing this, and sorry I didn't mention it on this list.
Greg
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply
* On --depth=funny value
From: Junio C Hamano @ 2013-01-09 2:53 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stefan Beller, Jonathan Nieder, schlotter, Ralf.Wildenhues, git
In-Reply-To: <CACsJy8BLLTWd+cTBj1jNW=ODPy7=Kg4-TPUdZ82YCE-0RQpMZA@mail.gmail.com>
Here to outline my current thinking. Note that this is unrelated to
the "git clone --bottom=v1.2.3" to say "I do not care about anything
that happened before that version".
* First, let's *not* do "git fetch --depth=inf"; if you want to
unplug the bottom of your shallow clone, be more explicit and
introduce a new option, e.g. "git fetch --unshallow", or
something.
* Make "git fetch" and "git clone" die() when zero or negative
number is given with --depth=$N, for the following reasons:
- The --depth option is describe as:
("git clone") ... a 'shallow' clone with a history
truncated to the specified number of revisions.
("git fetch") Limit fetching to ancestor-chains not longer
than n.
It is fairly clear from the above that negative $N does not
make any sense.
Technically, fetching the commits that were explicitly asked
for and not there parents is the only possible ancestor-chain
that is not longer than -4, so "git fetch --depth=-4" ought to
behave just like "git fetch --depth=0", but you have to be very
sick to read the documentation and expect things to work that
way. Also there is no way to misread the "git clone"
documentation and expect "git clone --depth=-4" to create a
history truncated to negative number of revisions.
Which means that it is the right thing to do to die() when a
negative number is given to --depth for these commands.
- As people count from one, the natural way to ask for the tip
commit without any history ought to be "--depth=1". Let's
declare the current behaviour of "--depth=1" that gives the tip
and one commit behind it a bug.
Which means that these commands should be updated to die() when
zero is given to their --depth option. "Do not give me any
history" is inherenty incompatibe with "clone" or "fetch".
Because there is no configuration variable "fetch.depth" (or
"clone.depth") that forces all your cloned repositories to be
shallow, "git clone --depth=0" or "git fetch --depth=0"
couldn't have been ways for existing users to ask to defeat any
funny configured depth value and clone or fetch everything.
When they wanted to clone or fetch everything, they would have
just used the command without any "--depth" option instead.
Which means that nobody gets hurt if we change these commands
to die() when zero is given to their --depth option.
* We would like to update "clone --depth=1" to end up with a tip
only repository, but let's not to touch "git fetch" (and "git
clone") and make them send 0 over the wire when the command line
tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
thing).
Instead, fix "upload-pack" (the bug is in get_shallow_commits()
in shallow.c, I think), so that it counts correctly. When the
other end asks for a history with 1-commit deep, it should return
a history that is 1-commit deep, and tell the other end about the
parents of the returned history, instead of returning a history
that is 2 commmits deep. So when talking with a newer server,
clients will get correct number of commits; when talkng with an
older server, clients will get a bit more than they asked, but
nothing will break.
Can people sanity check the reasoning outlined here? Anything I
missed?
The above outline identifies three concrete tasks that different
people can tackle more or less independently, each with updated
code, documentation and test:
1. "git fetch --unshallow" that gives a pretty surface on Duy's
"--depth=inf";
2. Making "git fetch" and "git clone" die on "--depth=0" or
"--depth=-4";
3 Updating "upload-pack" to count correctly.
I'll refrain from saying "Any takers?" for now.
^ permalink raw reply
* Re: On --depth=funny value
From: Duy Nguyen @ 2013-01-09 4:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jonathan Nieder, schlotter, Ralf.Wildenhues, git
In-Reply-To: <7vy5g383sy.fsf_-_@alter.siamese.dyndns.org>
On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> * First, let's *not* do "git fetch --depth=inf"; if you want to
> unplug the bottom of your shallow clone, be more explicit and
> introduce a new option, e.g. "git fetch --unshallow", or
> something.
No problem. "Something" could be --no-depth or --no-shallow. I think
any of them is better than --unshallow.
> * We would like to update "clone --depth=1" to end up with a tip
> only repository, but let's not to touch "git fetch" (and "git
> clone") and make them send 0 over the wire when the command line
> tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
> thing).
You can't anyway. Depth 0 on the wire is considered invalid by upload-pack.
> Instead, fix "upload-pack" (the bug is in get_shallow_commits()
> in shallow.c, I think), so that it counts correctly. When the
> other end asks for a history with 1-commit deep, it should return
> a history that is 1-commit deep, and tell the other end about the
> parents of the returned history, instead of returning a history
> that is 2 commmits deep. So when talking with a newer server,
> clients will get correct number of commits; when talkng with an
> older server, clients will get a bit more than they asked, but
> nothing will break.
I'll need to look at get_shallow_commits() anyway for the unshallow
patch. I'll probably do this too.
--
Duy
^ permalink raw reply
* Re: On --depth=funny value
From: Junio C Hamano @ 2013-01-09 5:19 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stefan Beller, Jonathan Nieder, schlotter, Ralf.Wildenhues, git
In-Reply-To: <CACsJy8CA-a0=HqTY9heJBhPO4M5jyLk=tf253rRKCRuTWz5teg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> * We would like to update "clone --depth=1" to end up with a tip
>> only repository, but let's not to touch "git fetch" (and "git
>> clone") and make them send 0 over the wire when the command line
>> tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
>> thing).
>
> You can't anyway. Depth 0 on the wire is considered invalid by upload-pack.
Yes, that is a good point that we say "if (0 < opt->depth) do the
shallow thing" everywhere, so 0 is spcial in that sense.
Which suggests that if we wanted to, we could update the fetch side
to do the off-by-one thing against the current upload-pack when the
given depth is two or more, and still send 1 when depth=1. When
talking with an updated upload-pack that advertises exact-shallow
protocol extension, it can disable that off-by-one for all values of
depth. That way, the updated client gets history of wrong depth
only for --depth=1 when talking with the current upload-pack; all
other cases, it will get history of correct depth.
Hmm?
^ permalink raw reply
* Re: [PATCH] commit: make default of "cleanup" option configurable
From: Jonathan Nieder @ 2013-01-09 7:29 UTC (permalink / raw)
To: Ralf Thielow; +Cc: git, gitster
In-Reply-To: <1357676176-30019-1-git-send-email-ralf.thielow@gmail.com>
Hi,
Ralf Thielow wrote:
> The default of the "cleanup" option in "git commit"
> is not configurable. Users who don't want to use the
> default have to pass this option on every commit since
> there's no way to configure it.
Could you give an example? I'm trying to get a sense of whether these
habitual --cleanup= passers would use --cleanup=verbatim or
--cleanup=strip.
I'm a bit worried that a setting like 'commit.cleanup = strip' would
be likely to break scripts. Yes, scripts using "git commit" instead
of commit-tree while caring about detailed semantics are asking for
trouble, but I'm worried about importers, for example, which are
sometimes written by new git users. Some scripts might assume that
"git commit" preserves the entire change description from their source
data, even when some lines happen to start by listing the ways the
author is #1.
I don't think that necessarily rules out this change, but:
1. We need more of an explanation of the purpose than "this lets
people type less". What workflow does setting this option fit
into?
2. I would prefer to see a little thought about whether it's possible
to avoid silently impacting scripts. E.g., depending on the
answer to (1), maybe supporting "verbatim" but not "strip" would be
ok? Or alternatively, maybe a search of public code repositories
would reveal that new git users tend to write their importers in a
way that this doesn't break.
As a side effect, the information gathered to answer (1) and (2) could
have the potential to make the user-level documentation more useful,
by adding context to the description of the configuration item.
[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -103,7 +103,7 @@ static enum {
> CLEANUP_NONE,
> CLEANUP_ALL
> } cleanup_mode;
> -static char *cleanup_arg;
> +const static char *cleanup_arg;
Style nit: storage class comes first, then the type. Otherwise the
typename "const char *" is split up.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH] commit: make default of "cleanup" option configurable
From: Ralf Thielow @ 2013-01-09 8:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, gitster
In-Reply-To: <20130109072952.GC6503@elie.Belkin>
Hi,
2013/1/9 Jonathan Nieder <jrnieder@gmail.com>:
> Hi,
>
> Ralf Thielow wrote:
>
>> The default of the "cleanup" option in "git commit"
>> is not configurable. Users who don't want to use the
>> default have to pass this option on every commit since
>> there's no way to configure it.
>
> Could you give an example? I'm trying to get a sense of whether these
> habitual --cleanup= passers would use --cleanup=verbatim or
> --cleanup=strip.
>
It's actually my own usecase :). The bugtracker I'm using is able
to create relationships between issues and related commits. It
expects that a part of the commit message contains the issue number
in format "#<issueId>". So I need to use a cleanup mode different
from "default" to keep the commentary. The mode I'd use is "whitespace",
"verbatim" is also possible.
> I'm a bit worried that a setting like 'commit.cleanup = strip' would
> be likely to break scripts. Yes, scripts using "git commit" instead
> of commit-tree while caring about detailed semantics are asking for
> trouble, but I'm worried about importers, for example, which are
> sometimes written by new git users. Some scripts might assume that
> "git commit" preserves the entire change description from their source
> data, even when some lines happen to start by listing the ways the
> author is #1.
>
When a user uses a script/importer which expects that the "default" option
is used without setting it explicitly, and then the user changes the default,
isn't it the users fault if that would break things?
> I don't think that necessarily rules out this change, but:
>
> 1. We need more of an explanation of the purpose than "this lets
> people type less". What workflow does setting this option fit
> into?
>
> 2. I would prefer to see a little thought about whether it's possible
> to avoid silently impacting scripts. E.g., depending on the
> answer to (1), maybe supporting "verbatim" but not "strip" would be
> ok? Or alternatively, maybe a search of public code repositories
> would reveal that new git users tend to write their importers in a
> way that this doesn't break.
>
> As a side effect, the information gathered to answer (1) and (2) could
> have the potential to make the user-level documentation more useful,
> by adding context to the description of the configuration item.
>
I'll add a sentence of my bugtracker example to it. I think many developers
are using such a tool, so it'd makes sense.
> [...]
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -103,7 +103,7 @@ static enum {
>> CLEANUP_NONE,
>> CLEANUP_ALL
>> } cleanup_mode;
>> -static char *cleanup_arg;
>> +const static char *cleanup_arg;
>
> Style nit: storage class comes first, then the type. Otherwise the
> typename "const char *" is split up.
>
Thanks.
I'll send a new version of the patch including changes of your and
Junios comments.
Ralf
^ permalink raw reply
* [PATCH 02/19] reset $pathspec: exit with code 0 if successful
From: Martin von Zweigbergk @ 2013-01-09 8:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
"git reset $pathspec" currently exits with a non-zero exit code if the
worktree is dirty after resetting, which is inconsistent with reset
without pathspec, and it makes it harder to know whether the command
really failed. Change it to exit with code 0 regardless of whether the
worktree is dirty so that non-zero indicates an error.
This makes the 4 "disambiguation" test cases in t7102 clearer since
they all used to "fail", 3 of which "failed" due to changes in the
work tree. Now only the ambiguous one fails.
---
I suppose this makes documenting the exit code unnecessary, since
"return zero iff successful" is probably understood to be the default.
The variable in refresh_index() containing the value to be returned is
called has_errors. I'm guessing I shouldn't take the name too
seriously.
builtin/reset.c | 8 +++-----
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 18 ++++++++++++------
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 8cc7c72..65413d0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -119,19 +119,17 @@ static void print_new_head_line(struct commit *commit)
static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
{
- int result;
-
if (!index_lock) {
index_lock = xcalloc(1, sizeof(struct lock_file));
fd = hold_locked_index(index_lock, 1);
}
- result = refresh_index(&the_index, (flags), NULL, NULL,
- _("Unstaged changes after reset:")) ? 1 : 0;
+ refresh_index(&the_index, (flags), NULL, NULL,
+ _("Unstaged changes after reset:"));
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(index_lock))
return error ("Could not refresh index");
- return result;
+ return 0;
}
static void update_index_from_diff(struct diff_queue_struct *q,
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 70edbb3..06b18f8 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -23,7 +23,7 @@ test_expect_success '"reset <submodule>" updates the index' '
git update-index --refresh &&
git diff-files --quiet &&
git diff-index --quiet --cached HEAD &&
- test_must_fail git reset HEAD^ submodule &&
+ git reset HEAD^ submodule &&
test_must_fail git diff-files --quiet &&
git reset submodule &&
git diff-files --quiet
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index b096dc8..81b2570 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -388,7 +388,8 @@ test_expect_success 'test --mixed <paths>' '
echo 4 > file4 &&
echo 5 > file1 &&
git add file1 file3 file4 &&
- test_must_fail git reset HEAD -- file1 file2 file3 &&
+ git reset HEAD -- file1 file2 file3 &&
+ test_must_fail git diff --quiet &&
git diff > output &&
test_cmp output expect &&
git diff --cached > output &&
@@ -402,7 +403,8 @@ test_expect_success 'test resetting the index at give paths' '
>sub/file2 &&
git update-index --add sub/file1 sub/file2 &&
T=$(git write-tree) &&
- test_must_fail git reset HEAD sub/file2 &&
+ git reset HEAD sub/file2 &&
+ test_must_fail git diff --quiet &&
U=$(git write-tree) &&
echo "$T" &&
echo "$U" &&
@@ -440,7 +442,8 @@ test_expect_success 'resetting specific path that is unmerged' '
echo "100644 $F3 3 file2"
} | git update-index --index-info &&
git ls-files -u &&
- test_must_fail git reset HEAD file2 &&
+ git reset HEAD file2 &&
+ test_must_fail git diff --quiet &&
git diff-index --exit-code --cached HEAD
'
@@ -449,7 +452,8 @@ test_expect_success 'disambiguation (1)' '
git reset --hard &&
>secondfile &&
git add secondfile &&
- test_must_fail git reset secondfile &&
+ git reset secondfile &&
+ test_must_fail git diff --quiet -- secondfile &&
test -z "$(git diff --cached --name-only)" &&
test -f secondfile &&
test ! -s secondfile
@@ -474,7 +478,8 @@ test_expect_success 'disambiguation (3)' '
>secondfile &&
git add secondfile &&
rm -f secondfile &&
- test_must_fail git reset HEAD secondfile &&
+ git reset HEAD secondfile &&
+ test_must_fail git diff --quiet &&
test -z "$(git diff --cached --name-only)" &&
test ! -f secondfile
@@ -486,7 +491,8 @@ test_expect_success 'disambiguation (4)' '
>secondfile &&
git add secondfile &&
rm -f secondfile &&
- test_must_fail git reset -- secondfile &&
+ git reset -- secondfile &&
+ test_must_fail git diff --quiet &&
test -z "$(git diff --cached --name-only)" &&
test ! -f secondfile
'
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 14/19] reset [--mixed]: don't write index file twice
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
When doing a mixed reset without paths, the index is locked, read,
reset, and written back as part of the actual reset operation (in
reset_index()). Then, when showing the list of worktree modifications,
we lock the index again, refresh it, and write it.
Change this so we only write the index once, making "git reset" a
little faster. It does mean that the index lock will be held a little
longer, but the difference is small compared to the time spent
refreshing the index.
There is one minor functional difference: We used to say "Could not
write new index file." if the first write failed, and "Could not
refresh index" if the second write failed. Now, we will only use the
first message.
This speeds up "git reset" a little on the linux-2.6 repo (best of
five, warm cache):
Before After
real 0m0.239s 0m0.214s
user 0m0.160s 0m0.130s
sys 0m0.070s 0m0.080s
---
builtin/reset.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 2243b95..254afa9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -335,6 +335,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
err = reset_index(sha1, MIXED, quiet);
if (err)
die(_("Could not reset index file to revision '%s'."), rev);
+
+ if (reset_type == MIXED) /* Report what has not been updated. */
+ update_index_refresh(
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock))
die(_("Could not write new index file."));
@@ -346,15 +351,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type == HARD && !update_ref_status && !quiet)
print_new_head_line(commit);
- else if (reset_type == MIXED) { /* Report what has not been updated. */
- struct lock_file *index_lock = xcalloc(1, sizeof(struct lock_file));
- int fd = hold_locked_index(index_lock, 1);
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
- if (write_cache(fd, active_cache, active_nr) ||
- commit_locked_index(index_lock))
- error("Could not refresh index");
- }
remove_branch_state();
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 04/19] reset: don't allow "git reset -- $pathspec" in bare repo
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
---
builtin/reset.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 045c960..664fad9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -295,8 +295,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
else if (reset_type != NONE)
die(_("Cannot do %s reset with paths."),
_(reset_type_names[reset_type]));
- return read_from_tree(pathspec, sha1,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
}
if (reset_type == NONE)
reset_type = MIXED; /* by default */
@@ -308,6 +306,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
+ if (pathspec)
+ return read_from_tree(pathspec, sha1,
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 15/19] reset.c: finish entire cmd_reset() whether or not pathspec is given
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
By not returning from inside the "if (pathspec)" block, we can let the
pathspec-aware and pathspec-less code share a bit more, making it
easier to make future changes that should affect both cases. This also
highlights the similarity between read_from_tree() and reset_index().
---
Should error reporting be aligned too? Speaking of which,
do_diff_cache() never returns anything by 0. Is the return value for
future-proofing?
builtin/reset.c | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 254afa9..9bcad29 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -308,19 +308,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
- if (pathspec) {
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- int index_fd = hold_locked_index(lock, 1);
- if (read_from_tree(pathspec, sha1))
- return 1;
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
- if (write_cache(index_fd, active_cache, active_nr) ||
- commit_locked_index(lock))
- return error("Could not write new index file.");
- return 0;
- }
-
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
@@ -330,11 +317,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type != SOFT) {
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int newfd = hold_locked_index(lock, 1);
- int err = reset_index(sha1, reset_type, quiet);
- if (reset_type == KEEP && !err)
- err = reset_index(sha1, MIXED, quiet);
- if (err)
- die(_("Could not reset index file to revision '%s'."), rev);
+ if (pathspec) {
+ if (read_from_tree(pathspec, sha1))
+ return 1;
+ } else {
+ int err = reset_index(sha1, reset_type, quiet);
+ if (reset_type == KEEP && !err)
+ err = reset_index(sha1, MIXED, quiet);
+ if (err)
+ die(_("Could not reset index file to revision '%s'."), rev);
+ }
if (reset_type == MIXED) /* Report what has not been updated. */
update_index_refresh(
@@ -345,14 +337,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("Could not write new index file."));
}
- /* Any resets update HEAD to the head being switched to,
- * saving the previous head in ORIG_HEAD before. */
- update_ref_status = update_refs(rev, sha1);
+ if (!pathspec) {
+ /* Any resets without paths update HEAD to the head being
+ * switched to, saving the previous head in ORIG_HEAD before. */
+ update_ref_status = update_refs(rev, sha1);
- if (reset_type == HARD && !update_ref_status && !quiet)
- print_new_head_line(commit);
+ if (reset_type == HARD && !update_ref_status && !quiet)
+ print_new_head_line(commit);
- remove_branch_state();
+ remove_branch_state();
+ }
return update_ref_status;
}
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 09/19] reset.c: replace switch by if-else
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
---
builtin/reset.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 42d1563..05ccfd4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -351,18 +351,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
* saving the previous head in ORIG_HEAD before. */
update_ref_status = update_refs(rev, sha1);
- switch (reset_type) {
- case HARD:
- if (!update_ref_status && !quiet)
- print_new_head_line(commit);
- break;
- case SOFT: /* Nothing else to do. */
- break;
- case MIXED: /* Report what has not been updated. */
+ if (reset_type == HARD && !update_ref_status && !quiet)
+ print_new_head_line(commit);
+ else if (reset_type == MIXED) /* Report what has not been updated. */
update_index_refresh(0, NULL,
quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
- break;
- }
remove_branch_state();
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 05/19] reset.c: extract function for parsing arguments
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Declutter cmd_reset() a bit by moving out the argument parsing to its
own function.
---
builtin/reset.c | 71 ++++++++++++++++++++++++++++++---------------------------
1 file changed, 38 insertions(+), 33 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 664fad9..9473725 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -198,36 +198,10 @@ static void die_if_unmerged_cache(int reset_type)
}
-int cmd_reset(int argc, const char **argv, const char *prefix)
-{
- int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
- int patch_mode = 0;
+const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) {
+ int i = 0;
const char *rev = "HEAD";
- unsigned char sha1[20], *orig = NULL, sha1_orig[20],
- *old_orig = NULL, sha1_old_orig[20];
- const char **pathspec = NULL;
- struct commit *commit;
- struct strbuf msg = STRBUF_INIT;
- const struct option options[] = {
- OPT__QUIET(&quiet, N_("be quiet, only report errors")),
- OPT_SET_INT(0, "mixed", &reset_type,
- N_("reset HEAD and index"), MIXED),
- OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),
- OPT_SET_INT(0, "hard", &reset_type,
- N_("reset HEAD, index and working tree"), HARD),
- OPT_SET_INT(0, "merge", &reset_type,
- N_("reset HEAD, index and working tree"), MERGE),
- OPT_SET_INT(0, "keep", &reset_type,
- N_("reset HEAD but keep local changes"), KEEP),
- OPT_BOOLEAN('p', "patch", &patch_mode, N_("select hunks interactively")),
- OPT_END()
- };
-
- git_config(git_default_config, NULL);
-
- argc = parse_options(argc, argv, prefix, options, git_reset_usage,
- PARSE_OPT_KEEP_DASHDASH);
-
+ unsigned char unused[20];
/*
* Possible arguments are:
*
@@ -250,7 +224,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
* Otherwise, argv[i] could be either <rev> or <paths> and
* has to be unambiguous.
*/
- else if (!get_sha1_committish(argv[i], sha1)) {
+ else if (!get_sha1_committish(argv[i], unused)) {
/*
* Ok, argv[i] looks like a rev; it should not
* be a filename.
@@ -262,6 +236,40 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
verify_filename(prefix, argv[i], 1);
}
}
+ *rev_ret = rev;
+ return i < argc ? get_pathspec(prefix, argv + i) : NULL;
+}
+
+int cmd_reset(int argc, const char **argv, const char *prefix)
+{
+ int reset_type = NONE, update_ref_status = 0, quiet = 0;
+ int patch_mode = 0;
+ const char *rev;
+ unsigned char sha1[20], *orig = NULL, sha1_orig[20],
+ *old_orig = NULL, sha1_old_orig[20];
+ const char **pathspec = NULL;
+ struct commit *commit;
+ struct strbuf msg = STRBUF_INIT;
+ const struct option options[] = {
+ OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+ OPT_SET_INT(0, "mixed", &reset_type,
+ N_("reset HEAD and index"), MIXED),
+ OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),
+ OPT_SET_INT(0, "hard", &reset_type,
+ N_("reset HEAD, index and working tree"), HARD),
+ OPT_SET_INT(0, "merge", &reset_type,
+ N_("reset HEAD, index and working tree"), MERGE),
+ OPT_SET_INT(0, "keep", &reset_type,
+ N_("reset HEAD but keep local changes"), KEEP),
+ OPT_BOOLEAN('p', "patch", &patch_mode, N_("select hunks interactively")),
+ OPT_END()
+ };
+
+ git_config(git_default_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, options, git_reset_usage,
+ PARSE_OPT_KEEP_DASHDASH);
+ pathspec = parse_args(argc, argv, prefix, &rev);
if (get_sha1_committish(rev, sha1))
die(_("Failed to resolve '%s' as a valid ref."), rev);
@@ -277,9 +285,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("Could not parse object '%s'."), rev);
hashcpy(sha1, commit->object.sha1);
- if (i < argc)
- pathspec = get_pathspec(prefix, argv + i);
-
if (patch_mode) {
if (reset_type != NONE)
die(_("--patch is incompatible with --{hard,mixed,soft}"));
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 19/19] reset [--mixed]: use diff-based reset whether or not pathspec was given
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Thanks to b65982b (Optimize "diff-index --cached" using cache-tree,
2009-05-20), resetting with paths is much faster than resetting
without paths. Some timings for the linux-2.6 repo to illustrate this
(best of five, warm cache):
reset reset .
real 0m0.219s 0m0.080s
user 0m0.140s 0m0.040s
sys 0m0.070s 0m0.030s
These two commands should do the same thing, so instead of having the
user type the trailing " ." to get the faster do_diff_cache()-based
implementation, always use it when doing a mixed reset, with or
without paths (so "git reset $rev" would also be faster).
Comparing before and after (best of five):
Before After
reset (warm cache): 0.21 0.07
reset -q (warm cache) 0.17 0.03
reset (cold cache): 10.31 2.72
reset -q (cold cache) 7.64 0.38
---
Are unmerged entries handled the same? read_from_tree() calls
read_cache(), while reset_index() calls read_cache_unmerged(). I
haven't figured out if/why they should be different.
Are there other differences, or could unpack_trees() learn the same
optimization as do_diff_cache()? Actually, the commit mentioned above
does say
Tweak unpack_trees() logic that is used to read in the tree object
to catch the case where the tree entry we are looking at matches the
index as a whole by looking at the cache-tree.
If there are differences, we are clearly missing tests for them. And
it seems like any difference between them should be fixed, so "git
reset" and "git reset ." (from root of tree) do the same thing even
before this patch.
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 5cd48ac..6db0a10 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -320,7 +320,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type != SOFT) {
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int newfd = hold_locked_index(lock, 1);
- if (pathspec) {
+ if (reset_type == MIXED) {
if (read_from_tree(pathspec, sha1))
return 1;
} else {
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 10/19] reset --keep: only write index file once
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
"git reset --keep" calls reset_index_file() twice, first doing a
two-way merge to the target revision, updating the index and worktree,
and then resetting the index. After each call, we write the index
file.
In the unlikely event that the second call to reset_index_file()
fails, the index will have been merged to the target revision, but
HEAD will not be updated, leaving the user with a dirty index.
By moving the locking, writing and committing out of
reset_index_file() and into the caller, we can avoid writing the index
twice, thereby making the sure we don't end up in the half-way reset
state. As a bonus, we speed up "git reset --keep" a little on the
linux-2.6 repo (best of five, warm cache):
Before After
real 0m0.315s 0m0.296s
user 0m0.290s 0m0.280s
sys 0m0.020s 0m0.010s
---
builtin/reset.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 05ccfd4..8e5d097 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -38,14 +38,12 @@ static inline int is_merge(void)
return !access(git_path("MERGE_HEAD"), F_OK);
}
-static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
+static int reset_index(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));
memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
@@ -67,8 +65,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
opts.reset = 1;
}
- newfd = hold_locked_index(lock, 1);
-
read_cache_unmerged();
if (reset_type == KEEP) {
@@ -91,10 +87,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
prime_cache_tree(&active_cache_tree, tree);
}
- if (write_cache(newfd, active_cache, active_nr) ||
- commit_locked_index(lock))
- return error(_("Could not write new index file."));
-
return 0;
}
@@ -340,9 +332,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die_if_unmerged_cache(reset_type);
if (reset_type != SOFT) {
- int err = reset_index_file(sha1, reset_type, quiet);
+ struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+ int newfd = hold_locked_index(lock, 1);
+ int err = reset_index(sha1, reset_type, quiet);
if (reset_type == KEEP && !err)
- err = reset_index_file(sha1, MIXED, quiet);
+ err = reset_index(sha1, MIXED, quiet);
+ if (!err &&
+ (write_cache(newfd, active_cache, active_nr) ||
+ commit_locked_index(lock))) {
+ err = error(_("Could not write new index file."));
+ }
if (err)
die(_("Could not reset index file to revision '%s'."), rev);
}
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 01/19] reset $pathspec: no need to discard index
From: Martin von Zweigbergk @ 2013-01-09 8:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Since 34110cd (Make 'unpack_trees()' have a separate source and
destination index, 2008-03-06), the index no longer gets clobbered by
do_diff_cache() and we can remove the code for discarding and
re-reading it.
There are two paths to update_index_refresh() from cmd_reset(), but on
both paths, either read_cache() or read_cache_unmerged() will have
been called, so the call to read_cache() in this method is redundant
(although practically free).
This speeds up "git reset -- ." a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.093s 0m0.080s
user 0m0.040s 0m0.020s
sys 0m0.050s 0m0.050s
---
builtin/reset.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 915cc9f..8cc7c72 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -126,9 +126,6 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
fd = hold_locked_index(index_lock, 1);
}
- if (read_cache() < 0)
- return error(_("Could not read index"));
-
result = refresh_index(&the_index, (flags), NULL, NULL,
_("Unstaged changes after reset:")) ? 1 : 0;
if (write_cache(fd, active_cache, active_nr) ||
@@ -141,12 +138,6 @@ static void update_index_from_diff(struct diff_queue_struct *q,
struct diff_options *opt, void *data)
{
int i;
- int *discard_flag = data;
-
- /* do_diff_cache() mangled the index */
- discard_cache();
- *discard_flag = 1;
- read_cache();
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
@@ -179,17 +170,15 @@ static int read_from_tree(const char *prefix, const char **argv,
unsigned char *tree_sha1, int refresh_flags)
{
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- int index_fd, index_was_discarded = 0;
+ int index_fd;
struct diff_options opt;
memset(&opt, 0, sizeof(opt));
diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), &opt);
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
- opt.format_callback_data = &index_was_discarded;
index_fd = hold_locked_index(lock, 1);
- index_was_discarded = 0;
read_cache();
if (do_diff_cache(tree_sha1, &opt))
return 1;
@@ -197,9 +186,6 @@ static int read_from_tree(const char *prefix, const char **argv,
diff_flush(&opt);
diff_tree_release_paths(&opt);
- if (!index_was_discarded)
- /* The index is still clobbered from do_diff_cache() */
- discard_cache();
return update_index_refresh(index_fd, lock, refresh_flags);
}
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 13/19] reset.c: move lock, write and commit out of update_index_refresh()
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
In preparation for the/a following patch, move the locking, writing
and committing of the index file out of update_index_refresh(). The
code duplication caused will soon be taken care of. What remains of
update_index_refresh() is just one line, but it is still called from
two places, so let's leave it for now.
In the process, we expose and fix the minor UI bug that makes us print
"Could not refresh index" when we fail to write the index file when
invoked with a pathspec. Copy the error message from the pathspec-less
codepath ("Could not write new index file.").
---
builtin/reset.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index a21ba31..2243b95 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,19 +109,10 @@ static void print_new_head_line(struct commit *commit)
printf("\n");
}
-static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
+static void update_index_refresh(int flags)
{
- if (!index_lock) {
- index_lock = xcalloc(1, sizeof(struct lock_file));
- fd = hold_locked_index(index_lock, 1);
- }
-
refresh_index(&the_index, (flags), NULL, NULL,
_("Unstaged changes after reset:"));
- if (write_cache(fd, active_cache, active_nr) ||
- commit_locked_index(index_lock))
- return error ("Could not refresh index");
- return 0;
}
static void update_index_from_diff(struct diff_queue_struct *q,
@@ -320,9 +311,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (pathspec) {
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int index_fd = hold_locked_index(lock, 1);
- return read_from_tree(pathspec, sha1) ||
- update_index_refresh(index_fd, lock,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (read_from_tree(pathspec, sha1))
+ return 1;
+ update_index_refresh(
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (write_cache(index_fd, active_cache, active_nr) ||
+ commit_locked_index(lock))
+ return error("Could not write new index file.");
+ return 0;
}
/* Soft reset does not touch the index file nor the working tree
@@ -350,9 +346,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type == HARD && !update_ref_status && !quiet)
print_new_head_line(commit);
- else if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(0, NULL,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ else if (reset_type == MIXED) { /* Report what has not been updated. */
+ struct lock_file *index_lock = xcalloc(1, sizeof(struct lock_file));
+ int fd = hold_locked_index(index_lock, 1);
+ update_index_refresh(
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (write_cache(fd, active_cache, active_nr) ||
+ commit_locked_index(index_lock))
+ error("Could not refresh index");
+ }
remove_branch_state();
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 00/19] reset improvements
From: Martin von Zweigbergk @ 2013-01-09 8:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
This is kind of a re-roll of [1] (wow, apparently it took me almost
two months to get done). The goal was, then and now, to teach "git
reset" to work on an unborn branch and to not require a commit when a
tree would do. This time, I also made some tangential improvements
along the way, mostly related to readability and performance.
As usual, the risker patches are towards the end. In particular, I
find it hard to evaluate how risky the last patch is. That last patch
is responsible for much of the improvements in the timing table below,
so it would be nice if it doesn't break things too badly (test pass,
of course). The timings are best-of-five, wall time.
Command Before After
reset (warm) 0.23 0.07
reset -q (warm) 0.23 0.03
reset . (warm) 0.09 0.07
reset -q . (warm) 0.09 0.03
reset --keep (warm) 0.31 0.29
reset --keep -q (warm) 0.31 0.29
reset (cold) 9.74 2.60
reset -q (cold) 9.85 0.37
reset . (cold) 2.66 2.51
reset -q . (cold) 2.59 0.33
reset --keep (cold) 7.58 7.52
reset --keep -q (cold) 7.37 7.21
[1] http://thread.gmane.org/gmane.comp.version-control.git/210568/focus=210855
Martin von Zweigbergk (19):
reset $pathspec: no need to discard index
reset $pathspec: exit with code 0 if successful
reset.c: pass pathspec around instead of (prefix, argv) pair
reset: don't allow "git reset -- $pathspec" in bare repo
reset.c: extract function for parsing arguments
reset.c: remove unnecessary variable 'i'
reset.c: extract function for updating {ORIG,}HEAD
reset.c: share call to die_if_unmerged_cache()
reset.c: replace switch by if-else
reset --keep: only write index file once
reset: avoid redundant error message
reset.c: move update_index_refresh() call out of read_from_tree()
reset.c: move lock, write and commit out of update_index_refresh()
reset [--mixed]: don't write index file twice
reset.c: finish entire cmd_reset() whether or not pathspec is given
reset [--mixed] --quiet: don't refresh index
reset $sha1 $pathspec: require $sha1 only to be treeish
reset: allow reset on unborn branch
reset [--mixed]: use diff-based reset whether or not pathspec was
given
builtin/reset.c | 281 +++++++++++++++++++----------------------
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 26 +++-
t/t7106-reset-unborn-branch.sh | 52 ++++++++
4 files changed, 200 insertions(+), 161 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply
* [PATCH 17/19] reset $sha1 $pathspec: require $sha1 only to be treeish
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Resetting with paths does not update HEAD and there is nothing else
that a commit should be needed for. Relax the argument parsing so only
a tree is required.
The sha1 is only passed to read_from_tree(), which already only
requires a tree.
The "rev" variable we pass to run_add_interactive() will resolve to a
tree. This is fine since interactive_reset only needs the parameter to
be a treeish and doesn't use it for display purposes.
---
Is it correct that interactive_reset does not use the revision
specifier for display purposes? Or, worse, that it requires it to be a
commit in some cases? I tried it and didn't see any problem.
Can the two blocks of code that look up commit or tree be made to
share more? I'm not very familiar with what functions are available. I
think I tried keeping a separate "struct object *object" to be able to
put the last three lines outside the blocks, but didn't like the
result.
builtin/reset.c | 46 ++++++++++++++++++++++++++--------------------
t/t7102-reset.sh | 8 ++++++++
2 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index a2e69eb..4c223bd 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -177,9 +177,10 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c
/*
* Possible arguments are:
*
- * git reset [-opts] <rev> <paths>...
- * git reset [-opts] <rev> -- <paths>...
- * git reset [-opts] -- <paths>...
+ * git reset [-opts] [<rev>]
+ * git reset [-opts] <tree> [<paths>...]
+ * git reset [-opts] <tree> -- [<paths>...]
+ * git reset [-opts] -- [<paths>...]
* git reset [-opts] <paths>...
*
* At this point, argv points immediately after [-opts].
@@ -194,11 +195,13 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c
}
/*
* Otherwise, argv[0] could be either <rev> or <paths> and
- * has to be unambiguous.
+ * has to be unambiguous. If there is a single argument, it
+ * can not be a tree
*/
- else if (!get_sha1_committish(argv[0], unused)) {
+ else if ((argc == 1 && !get_sha1_committish(argv[0], unused)) ||
+ (argc > 1 && !get_sha1_treeish(argv[0], unused))) {
/*
- * Ok, argv[0] looks like a rev; it should not
+ * Ok, argv[0] looks like a commit/tree; it should not
* be a filename.
*/
verify_non_filename(prefix, argv[0]);
@@ -240,7 +243,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
const char *rev;
unsigned char sha1[20];
const char **pathspec = NULL;
- struct commit *commit;
+ struct commit *commit = NULL;
const struct option options[] = {
OPT__QUIET(&quiet, N_("be quiet, only report errors")),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -262,19 +265,22 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_DASHDASH);
pathspec = parse_args(argc, argv, prefix, &rev);
- if (get_sha1_committish(rev, sha1))
- die(_("Failed to resolve '%s' as a valid ref."), rev);
-
- /*
- * NOTE: As "git reset $treeish -- $path" should be usable on
- * any tree-ish, this is not strictly correct. We are not
- * moving the HEAD to any commit; we are merely resetting the
- * entries in the index to that of a treeish.
- */
- commit = lookup_commit_reference(sha1);
- if (!commit)
- die(_("Could not parse object '%s'."), rev);
- hashcpy(sha1, commit->object.sha1);
+ if (!pathspec) {
+ if (get_sha1_committish(rev, sha1))
+ die(_("Failed to resolve '%s' as a valid revision."), rev);
+ commit = lookup_commit_reference(sha1);
+ if (!commit)
+ die(_("Could not parse object '%s'."), rev);
+ hashcpy(sha1, commit->object.sha1);
+ } else {
+ struct tree *tree;
+ if (get_sha1_treeish(rev, sha1))
+ die(_("Failed to resolve '%s' as a valid tree."), rev);
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_("Could not parse object '%s'."), rev);
+ hashcpy(sha1, tree->object.sha1);
+ }
if (patch_mode) {
if (reset_type != NONE)
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 81b2570..1fa2a5f 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -497,4 +497,12 @@ test_expect_success 'disambiguation (4)' '
test ! -f secondfile
'
+test_expect_success 'reset with paths accepts tree' '
+ # for simpler tests, drop last commit containing added files
+ git reset --hard HEAD^ &&
+ git reset HEAD^^{tree} -- . &&
+ git diff --cached HEAD^ --exit-code &&
+ git diff HEAD --exit-code
+'
+
test_done
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 08/19] reset.c: share call to die_if_unmerged_cache()
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Use a single condition to guard the call to die_if_unmerged_cache for
both --soft and --keep. This avoids the small distraction of the
precondition check from the logic following it.
Also change an instance of
if (e)
err = err || f();
to the almost as short, but clearer
if (e && !err)
err = f();
(which is equivalent since we only care whether exit code is 0)
---
builtin/reset.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 4d556e7..42d1563 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -336,15 +336,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
- if (reset_type == SOFT)
+ if (reset_type == SOFT || reset_type == KEEP)
die_if_unmerged_cache(reset_type);
- else {
- int err;
- if (reset_type == KEEP)
- die_if_unmerged_cache(reset_type);
- err = reset_index_file(sha1, reset_type, quiet);
- if (reset_type == KEEP)
- err = err || reset_index_file(sha1, MIXED, quiet);
+
+ if (reset_type != SOFT) {
+ int err = reset_index_file(sha1, reset_type, quiet);
+ if (reset_type == KEEP && !err)
+ err = reset_index_file(sha1, MIXED, quiet);
if (err)
die(_("Could not reset index file to revision '%s'."), rev);
}
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
"git reset [--mixed]" without --quiet refreshes the index in order to
display the "Unstaged changes after reset". When --quiet is given,
that output is suppressed, removing the need to refresh the index.
Other porcelain commands that care about a refreshed index should
already be refreshing it, so running e.g. "git reset -q && git diff"
is still safe.
This commit together with 686b2de (oneway_merge(): only lstat() when
told to update worktree, 2012-12-20) removes all calls to lstat() the
worktree from the command.
This speeds up "git reset -q" a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.215s 0m0.176s
user 0m0.150s 0m0.130s
sys 0m0.060s 0m0.040s
And with cold cache (best of five):
Before After
real 0m11.351s 0m8.420s
user 0m0.230s 0m0.220s
sys 0m0.270s 0m0.060s
---
There is a test case in t7102 called '--mixed refreshes the index',
but it only checks that right output it printed. Is the test case not
testing right or not named right? As you can see, I suspect it's the
name/description that should change.
builtin/reset.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 9bcad29..a2e69eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,12 +109,6 @@ static void print_new_head_line(struct commit *commit)
printf("\n");
}
-static void update_index_refresh(int flags)
-{
- refresh_index(&the_index, (flags), NULL, NULL,
- _("Unstaged changes after reset:"));
-}
-
static void update_index_from_diff(struct diff_queue_struct *q,
struct diff_options *opt, void *data)
{
@@ -328,9 +322,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("Could not reset index file to revision '%s'."), rev);
}
- if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (reset_type == MIXED && !quiet) /* Report what has not been updated. */
+ refresh_index(&the_index, REFRESH_IN_PORCELAIN, NULL, NULL,
+ _("Unstaged changes after reset:"));
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock))
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
* [PATCH 07/19] reset.c: extract function for updating {ORIG,}HEAD
From: Martin von Zweigbergk @ 2013-01-09 8:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
By extracting the code for updating the HEAD and ORIG_HEAD symbolic
references to a separate function, we declutter cmd_reset() a bit and
we make it clear that e.g. the four variables {,sha1_}{,old_}orig are
only used by this code.
---
builtin/reset.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 68be05c..4d556e7 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -239,16 +239,35 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c
return *argv ? get_pathspec(prefix, argv) : NULL;
}
+static int update_refs(const char *rev, const unsigned char *sha1) {
+ int update_ref_status;
+ struct strbuf msg = STRBUF_INIT;
+ unsigned char *orig = NULL, sha1_orig[20],
+ *old_orig = NULL, sha1_old_orig[20];
+
+ if (!get_sha1("ORIG_HEAD", sha1_old_orig))
+ old_orig = sha1_old_orig;
+ if (!get_sha1("HEAD", sha1_orig)) {
+ orig = sha1_orig;
+ set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
+ update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+ }
+ else if (old_orig)
+ delete_ref("ORIG_HEAD", old_orig, 0);
+ set_reflog_message(&msg, "updating HEAD", rev);
+ update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+ strbuf_release(&msg);
+ return update_ref_status;
+}
+
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = NONE, update_ref_status = 0, quiet = 0;
int patch_mode = 0;
const char *rev;
- unsigned char sha1[20], *orig = NULL, sha1_orig[20],
- *old_orig = NULL, sha1_old_orig[20];
+ unsigned char sha1[20];
const char **pathspec = NULL;
struct commit *commit;
- struct strbuf msg = STRBUF_INIT;
const struct option options[] = {
OPT__QUIET(&quiet, N_("be quiet, only report errors")),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -332,17 +351,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
/* Any resets update HEAD to the head being switched to,
* saving the previous head in ORIG_HEAD before. */
- if (!get_sha1("ORIG_HEAD", sha1_old_orig))
- old_orig = sha1_old_orig;
- if (!get_sha1("HEAD", sha1_orig)) {
- orig = sha1_orig;
- set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
- update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
- }
- else if (old_orig)
- delete_ref("ORIG_HEAD", old_orig, 0);
- set_reflog_message(&msg, "updating HEAD", rev);
- update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+ update_ref_status = update_refs(rev, sha1);
switch (reset_type) {
case HARD:
@@ -359,7 +368,5 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
remove_branch_state();
- strbuf_release(&msg);
-
return update_ref_status;
}
--
1.8.1.rc3.331.g1ef2165
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox