Re: Proposal: create meaningful aliases for git reset's hard/soft/mixed
On Wed, Nov 23, 2011 at 10:51 AM, Junio C Hamano gits...@pobox.com wrote: I am guilty of introducing git reset --soft HEAD^ before I invented commit --amend during v1.3.0 timeframe to solve the issue soft reset originally wanted to. I do use commit --amend a lot, but I still appreciate having reset --soft. For example, to squash the last few commits: git reset --soft HEAD^^^ git commit --amend or undo commit --amend: git reset --soft HEAD@{1} git commit --amend Maybe there's a better way of doing that? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: [RFC/FR] Should git checkout (-B|-b) branch master...branch work?
Oops, meant for all of you. -- Forwarded message -- From: Martin von Zweigbergk martinv...@gmail.com Date: Fri, Dec 21, 2012 at 8:45 AM Subject: Re: [RFC/FR] Should git checkout (-B|-b) branch master...branch work? To: Junio C Hamano gits...@pobox.com On Fri, Dec 21, 2012 at 7:58 AM, Junio C Hamano gits...@pobox.com wrote: $ git checkout -B branch old fork point Unfortunately, master...branch syntax does not seem to work for specifying the old fork point for this purpose I have personally always found it confusing to use the same syntax for specifying ranges/sets and single revisions. I keep forgetting what git diff A..B does. I know it doesn't do what I expect (i.e. git diff $(git merge-base A B) B), but I don't know what it does (maybe same as git diff A B (?), but that's besides the point). Having worked a bit on rebase, I know that $onto can also take the A...B form. So there is clearly some precedence for the ... syntax to refer to a revision in some contexts. I would have much preferred if it was possible to make the revision parser generally interpret e.g. A.^.B as the merge base of A and B (failing if not exactly one). It seems like something that must have come up before. Is there a particular reason this would not be a good idea? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] tests: move test_cmp_rev to test-lib-functions
A function for checking that two given parameters refer to the same revision was defined in several places, so move the definition to test-lib-functions.sh instead. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- t/t1505-rev-parse-last.sh | 18 +- t/t3404-rebase-interactive.sh | 6 -- t/t3507-cherry-pick-conflict.sh | 6 -- t/t3508-cherry-pick-many-commits.sh | 8 ++-- t/t3510-cherry-pick-sequence.sh | 6 -- t/t6030-bisect-porcelain.sh | 4 +--- t/test-lib-functions.sh | 7 +++ 7 files changed, 15 insertions(+), 40 deletions(-) diff --git a/t/t1505-rev-parse-last.sh b/t/t1505-rev-parse-last.sh index d709ecf..4969edb 100755 --- a/t/t1505-rev-parse-last.sh +++ b/t/t1505-rev-parse-last.sh @@ -32,32 +32,24 @@ test_expect_success 'setup' ' # # and 'side' should be the last branch -test_rev_equivalent () { - - git rev-parse $1 expect - git rev-parse $2 output - test_cmp expect output - -} - test_expect_success '@{-1} works' ' - test_rev_equivalent side @{-1} + test_cmp_rev side @{-1} ' test_expect_success '@{-1}~2 works' ' - test_rev_equivalent side~2 @{-1}~2 + test_cmp_rev side~2 @{-1}~2 ' test_expect_success '@{-1}^2 works' ' - test_rev_equivalent side^2 @{-1}^2 + test_cmp_rev side^2 @{-1}^2 ' test_expect_success '@{-1}@{1} works' ' - test_rev_equivalent side@{1} @{-1}@{1} + test_cmp_rev side@{1} @{-1}@{1} ' test_expect_success '@{-2} works' ' - test_rev_equivalent master @{-2} + test_cmp_rev master @{-2} ' test_expect_success '@{-3} fails' ' diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 32fdc99..8462be1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -29,12 +29,6 @@ Initial setup: . $TEST_DIRECTORY/lib-rebase.sh -test_cmp_rev () { - git rev-parse --verify $1 expect.rev - git rev-parse --verify $2 actual.rev - test_cmp expect.rev actual.rev -} - set_fake_editor # WARNING: Modifications to the initial repository can change the SHA ID used diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index c82f721..223b984 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -11,12 +11,6 @@ test_description='test cherry-pick and revert with conflicts . ./test-lib.sh -test_cmp_rev () { - git rev-parse --verify $1 expect.rev - git rev-parse --verify $2 actual.rev - test_cmp expect.rev actual.rev -} - pristine_detach () { git checkout -f $1^0 git read-tree -u --reset HEAD diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 340afc7..4e7136b 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -5,15 +5,11 @@ test_description='test cherry-picking many commits' . ./test-lib.sh check_head_differs_from() { - head=$(git rev-parse --verify HEAD) - arg=$(git rev-parse --verify $1) - test $head != $arg + ! test_cmp_rev HEAD $1 } check_head_equals() { - head=$(git rev-parse --verify HEAD) - arg=$(git rev-parse --verify $1) - test $head = $arg + test_cmp_rev HEAD $1 } test_expect_success setup ' diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index b5fb527..7b7a89d 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -24,12 +24,6 @@ pristine_detach () { git clean -d -f -f -q -x } -test_cmp_rev () { - git rev-parse --verify $1 expect.rev - git rev-parse --verify $2 actual.rev - test_cmp expect.rev actual.rev -} - test_expect_success setup ' git config advice.detachedhead false echo unrelated unrelated diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 72e28ee..3e0e15f 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -676,9 +676,7 @@ test_expect_success 'bisect fails if tree is broken on trial commit' ' check_same() { echo Checking $1 is the same as $2 - git rev-parse $1 expected.same - git rev-parse $2 expected.actual - test_cmp expected.same expected.actual + test_cmp_rev $1 $2 } test_expect_success 'bisect: --no-checkout - start commit bad' ' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 22a4f8f..fa62d01 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -602,6 +602,13 @@ test_cmp() { $GIT_TEST_CMP $@ } +# Tests that its two parameters refer to the same revision +test_cmp_rev () { + git rev-parse --verify $1 expect.rev + git rev-parse --verify $2 actual.rev + test_cmp expect.rev actual.rev +} + # Print a sequence of numbers or letters in increasing order. This is # similar to GNU seq(1), but the latter
[PATCH 2/2] learn to pick/revert into unborn branch
From the user's point of view, it seems natural to think that cherry-picking into an unborn branch should work, so make it work, with or without --ff. Cherry-picking anything other than a commit that only adds files, will naturally result in conflicts. Similarly, revert also works, but will result in conflicts unless the specified revision only deletes files. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- The plan is to use this for fixing git rebase --root as discussed in http://thread.gmane.org/gmane.comp.version-control.git/205796 Is there a better way of creating an unborn branch than what I do in the test cases? sequencer.c | 19 +++ t/t3501-revert-cherry-pick.sh | 9 + t/t3506-cherry-pick-ff.sh | 8 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index 2260490..1ac1ceb 100644 --- a/sequencer.c +++ b/sequencer.c @@ -186,14 +186,15 @@ static int error_dirty_index(struct replay_opts *opts) return -1; } -static int fast_forward_to(const unsigned char *to, const unsigned char *from) +static int fast_forward_to(const unsigned char *to, const unsigned char *from, + int unborn) { struct ref_lock *ref_lock; read_cache(); if (checkout_fast_forward(from, to, 1)) exit(1); /* the callee should have complained already */ - ref_lock = lock_any_ref_for_update(HEAD, from, 0); + ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0); return write_ref_sha1(ref_lock, to, cherry-pick); } @@ -390,7 +391,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) struct commit_message msg = { NULL, NULL, NULL, NULL, NULL }; char *defmsg = NULL; struct strbuf msgbuf = STRBUF_INIT; - int res; + int res, unborn = 0; if (opts-no_commit) { /* @@ -402,9 +403,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (write_cache_as_tree(head, 0, NULL)) die (_(Your index file is unmerged.)); } else { - if (get_sha1(HEAD, head)) - return error(_(You do not have a valid HEAD)); - if (index_differs_from(HEAD, 0)) + unborn = get_sha1(HEAD, head); + if (unborn) + hashcpy(head, EMPTY_TREE_SHA1_BIN); + if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : HEAD, 0)) return error_dirty_index(opts); } discard_cache(); @@ -435,8 +437,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) else parent = commit-parents-item; - if (opts-allow_ff parent !hashcmp(parent-object.sha1, head)) - return fast_forward_to(commit-object.sha1, head); + if (opts-allow_ff + (parent !hashcmp(parent-object.sha1, head) || !parent unborn)) +return fast_forward_to(commit-object.sha1, head, unborn); if (parent parse_commit(parent) 0) /* TRANSLATORS: The first %s will be revert or diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 34c86e5..6f489e2 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -100,4 +100,13 @@ test_expect_success 'revert forbidden on dirty working tree' ' ' +test_expect_success 'chery-pick on unborn branch' ' + git checkout --orphan unborn + git rm --cached -r . + rm -rf * + git cherry-pick initial + git diff --quiet initial + ! test_cmp_rev initial HEAD +' + test_done diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh index 51ca391..373aad6 100755 --- a/t/t3506-cherry-pick-ff.sh +++ b/t/t3506-cherry-pick-ff.sh @@ -105,4 +105,12 @@ test_expect_success 'cherry pick a root commit with --ff' ' test $(git rev-parse --verify HEAD) = 1df192cd8bc58a2b275d842cede4d221ad9000d1 ' +test_expect_success 'chery-pick --ff on unborn branch' ' + git checkout --orphan unborn + git rm --cached -r . + rm -rf * + git cherry-pick --ff first + test_cmp_rev first HEAD +' + test_done -- 1.8.0.1.240.ge8a1f5a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] learn to pick/revert into unborn branch
On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: From the user's point of view, it seems natural to think that cherry-picking into an unborn branch should work, so make it work, with or without --ff. I actually am having a hard time imagining how that could ever be natural. Fair enough. What's natural is of course very subjective. In my opinion, whenever possible, operations on an unborn branch should behave exactly as they would on an arbitrary commit whose tree just happens to be empty. Of course, pretty much any operation that needs more than the tree (indirectly) pointed to by HEAD would fail the whenever possible clause. I realize that cherry-pick _does_ need the current commit to record the parent of the resulting commit, but that almost seems like an implementation detail, i.e whether we're adding a parent or adding no parent (when on unborn branch) to the list of parents. In the same way, I think git reset should work on an unborn branch, even though there is no commit that we can be modifying index and working tree to match (from man page). I think many users, like me, think of unborn branches as having an empty tree, rather than being a special state before history is created. Sure, such thinking is not technically correct, but it still seems to be some people's intuition (including mine). Cherry-picking anything other than a commit that only adds files, will naturally result in conflicts. Similarly, revert also works, but will result in conflicts unless the specified revision only deletes files. You may be able to make it work for some definition of work, but I am not sure how useful it is. As for use cases, I didn't consider that much more than that it might be useful for implementing git rebase --root. I haven't implemented that yet, so I can't say for sure that it will work out. One use case might be to rewrite history by creating an new unborn branch and picking the initial commit and a subset of other commits. Anyway, I didn't implement it because I thought it would be very useful, but mostly because I just thought it should work (for completeness). I could resend as part of my rebase series (called mz/rebase-range at some point) once that's done. Then we can discuss another solution in the scope of that series if we don't agree on allowing on cherry-pick on an unborn branch. Btw, I have another series, which I'll send after 1.8.1, that teaches git reset to work on an unborn branch (among other things). We might want to decide to support both or neither of the commands on an unborn branch. On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: From the user's point of view, it seems natural to think that cherry-picking into an unborn branch should work, so make it work, with or without --ff. I actually am having a hard time imagining how that could ever be natural. When you are on an unborn branch, you may have some files in your working tree, and some of them may even be registered to the index, but the index is merely for your convenience to create your first commit, and as far as the history is concered, it does not matter. By definition you do not have any history in such a state. What does it even mean to cherry-pick another commit, especially without the --no-commit option? The resulting commit will carry the message taken from the original commit, but does what it says match what you have done? I can understand that it may sometimes make sense to do $ git show --diff-filter=A $that_commit | git apply as a way to further update the uncommitted state you have in the working tree, so I can sort of buy that --no-commit case might make some sense (but if you make a commit after cherry-pick --no-commit, you still get the log message from that commit, which does not explain the other things you have in your working tree) in a limited situation. It seems to me that the only case that may make sense is to grab the contents from an existing tree, which might be better served with $ git checkout $that_commit -- $these_paths_I_am_interested_in Cherry-picking anything other than a commit that only adds files, will naturally result in conflicts. Similarly, revert also works, but will result in conflicts unless the specified revision only deletes files. You may be able to make it work for some definition of work, but I am not sure how useful it is. Puzzled... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] learn to pick/revert into unborn branch
On Sun, Dec 23, 2012 at 11:18 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano gits...@pobox.com wrote: I am not opposed to an internal use of the cherry-pick machinery to implement a corner case of rebase -i: 3. You run rebase -i --root to get this insn sheet: pick Add Makefile and hello.c for Hello world pick Add goodbye.c for Goodbye world and swap them: pick Add goodbye.c for Goodbye world pick Add Makefile and hello.c for Hello world Right, and as you point out in your later message, editing the initial commit is another (and more useful) use case. It could of course be special cased in git rebase, but I think doing it git cherry-pick is the right thing to do. Hopefully git-rebase.sh can then just reset to the void state and git-rebase--interactive.sh can just continue without knowing or caring whether it got started on an unborn branch. Hmm... I just realized the branch in unborn branch really means we don't have unborn detached HEAD, do we? So some more tricks are probably necessary after all. :-( [...] It transplants something that used to depend on the entire history behind it to be the beginning of the history so its log needs to be adjusted, but rebase -i can choose to always make it conflict and force the user to write a correct log message, so it won't expose the fundamental flaw you would add if you allowed the end-user facing cherry-pick to pick something to create a new root commit without interaction. If I understand you correctly, you are suggesting that git rebase should set the action from pick to edit for the first commit in the insn sheet if it is not a root commit. git rebase -i --root doesn't currently do that, but it certainly could. I agree that git reset without any commit parameter to reset the index and optionally the working tree (with --hard) should reset from an empty tree when you do not yet have any commit. Good to hear. But I do not think it has anything to do with cherry-pick to empty, so I do not agree with In the same way at all. See later comment. One use case might be to rewrite history by creating an new unborn branch and picking the initial commit and a subset of other commits. If you mean, in the above sample history, to git cherry-pick the commit that starts the Hello world and then do something else on top of the resulting history, how would that be different from forking from that existing root commit? True, the result would be the same. The user's thought process might be a little different (let me start from scratch vs let me start almost from scratch), but that's a very minor difference that I'm sure any user would quickly overcome. Anyway, I didn't implement it because I thought it would be very useful, but mostly because I just thought it should work (for completeness). I would not exactly call X complete if X works in one way in most cases and it works in quite a different way in one other case, only because it would have to barf if it wanted to work in the same way as in most cases, and the different behaviour is chosen only because X that does something is better than X that stops in an impossible situation and barfs. I agree, of course, but I don't see the behavior as different. When thinking about behavior around the root of the history, I imagine that all root commits actually have a parent, and that they all have the same parent. I also imagine that on an unborn branch, instead of being invalid, HEAD points to that same single root commit with an empty tree. Despite this model not matching git's, I find that this helps me reason about what the behavior of various commands should be. With this reasoning, cherry-picking into an unborn branch is no different from cherry-picking into any commit with an empty tree (which of course would be rare, but not forbidden). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Find the starting point of a local branch
On Sun, Dec 23, 2012 at 11:31 PM, Woody Wu narkewo...@gmail.com wrote: On Sun, Dec 23, 2012 at 11:09:58PM -0500, Seth Robertson wrote: In message 20121224035825.GA17203@zuhnb712, Woody Wu writes: How can I find out what's the staring reference point (a commit number or tag name) of a locally created branch? I can use gitk to find out it but this method is slow, I think there might be a command line to do it quickly. The answer is more complex than you probably suspected. Technically, `git log --oneline mybranch | tail -n 1` will tell you the starting point of any branch. But...I'm sure that isn't what you want to know. You want to know what commit was I at when I typed `git branch mybranch`? Yes, this is exactly I want to know. The problem is git doesn't record this information and doesn't have the slightest clue. But, you say, I can use `gitk` and see it. See? Right there. That isn't (necessarily) the starting point of the branch, it is the place where your branch diverged from some other branch. Git is actually quite able to tell you when the last time your branch diverged from some other branch. `git merge-base mybranch master` will tell you this, and is probably the answer you were looking for. This is not working to me since I have more than one local branch that diverged from the master, and in fact, the branch I have in question was diverged from another local branch. As Jeff mentions in a later message, git pull --rebase would probably do what you want. It works with local branches too. I once tried to add the same cleverness that git pull --rebase directly in git rebase [1], but there were several issues with those patches, one of was regarding the performance (git pull --rebase can be equally slow, but since it often involves network, users probably rarely notice). I think it would be nice to at least add it as an option to git rebase some day. Until then, git pull --rebase works fine. [1] http://thread.gmane.org/gmane.comp.version-control.git/166710 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Find the starting point of a local branch
On Thu, Dec 27, 2012 at 9:15 PM, Woody Wu narkewo...@gmail.com wrote: On Mon, Dec 24, 2012 at 09:24:39AM -0800, Martin von Zweigbergk wrote: On Sun, Dec 23, 2012 at 11:31 PM, Woody Wu narkewo...@gmail.com wrote: This is not working to me since I have more than one local branch that diverged from the master, and in fact, the branch I have in question was diverged from another local branch. As Jeff mentions in a later message, git pull --rebase would probably do what you want. It works with local branches too. I think what 'git pull --rebase' would do is to fetch from the origin and do a 'git rebase'. Not if the configured upstream is a local branch (see the branch.name.* configuration variables). In that case it will just rebase the local branch onto the new position of its upstream. If the upstream is not configured, I believe you can still do git pull --rebase . upstream branch. On one hand, I don't understand 'git rebase' so much from the manual, ont the other hand, I did not get the point why 'git rebase' has something to do with the thing I want to do (what I want is just query some kind of history information). I may have misunderstood or assumed things incorrectly that you wanted to rebase the commits on your branch. So why do you want to know? (Please ignore me if this was answered elsewhere in the thread that I might have missed.) Anyway, to answer your question, you could use a method similar to what git pull --rebase uses internally to figure out the branch point: git merge-base $(git rev-parse branch) $(git rev-list -g upstream branch) Hope that helps -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Makefile dependency from 'configure' to 'GIT-VERSION-FILE'
Hi, I use autoconf with git.git. I have noticed lately, especially when doing things like git rebase -i --exec make, that ./configure is run every time. If I understand correctly, this is because of 8242ff4 (build: reconfigure automatically if configure.ac changes, 2012-07-19). Just a few days before that commit, on 2012-07-15, the branch jn/makefile-cleanup including 520a6cd (Makefile: move GIT-VERSION-FILE dependencies closer to use, 2012-06-20) was merged (to next?). I wonder if these two subjects were aware of each other. The reason 'configure' depends on GIT-VERSION-FILE is because it inserts the version into the call to AC_INIT. I have close to no experience with autoconf or even make and it's not at all clear to me why we need to pass the verison to AC_INIT. It seems like it's just for messages printed by ./configure. If that's the case, we shouldn't need to generate a new 'configure' file ever time. At the very least, we shouldn't need to run it. Do you think we should simply remove the dependency from 'configure' to 'GIT-VERSION-FILE' and leave a comment there instead? Or should we instead somehow make 'reconfigure' depend only on 'configure.ac'? Both of these feel a little wrong to me, because they would remove real dependencies. Maybe the (probably mangled) patch at the end of this message is better? Martin diff --git a/Makefile b/Makefile index 736ecd4..ec5d7ca 100644 --- a/Makefile +++ b/Makefile @@ -2267,12 +2267,9 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : unimplemented.sh mv $@+ $@ endif # NO_PYTHON -configure: configure.ac GIT-VERSION-FILE - $(QUIET_GEN)$(RM) $@ $+ \ - sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ - $ $+ \ - autoconf -o $@ $+ \ - $(RM) $+ +configure: configure.ac + $(QUIET_GEN)$(RM) $@ \ + autoconf -o $@ $ ifdef AUTOCONFIGURED config.status: configure diff --git a/configure.ac b/configure.ac index ad215cc..00c3e38 100644 --- a/configure.ac +++ b/configure.ac @@ -142,7 +142,10 @@ fi ## Configure body starts here. AC_PREREQ(2.59) -AC_INIT([git], [@@GIT_VERSION@@], [git@vger.kernel.org]) +AC_INIT([git], + m4_esyscmd([ ./GIT-VERSION-GEN +{ sed -ne 's/GIT_VERSION = //p' GIT-VERSION-FILE | xargs echo -n; } ]), + [git@vger.kernel.org]) AC_CONFIG_SRCDIR([git.c]) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Makefile dependency from 'configure' to 'GIT-VERSION-FILE'
On Tue, Jan 1, 2013 at 11:21 PM, Jonathan Nieder jrnie...@gmail.com wrote: How about this patch (untested)? Looks good. Thanks! --- a/Makefile +++ b/Makefile @@ -2267,12 +2267,9 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : unimplemented.sh mv $@+ $@ endif # NO_PYTHON -configure: configure.ac GIT-VERSION-FILE +configure: configure.ac [...] --- a/configure.ac +++ b/configure.ac @@ -142,7 +142,10 @@ fi ## Configure body starts here. AC_PREREQ(2.59) -AC_INIT([git], [@@GIT_VERSION@@], [git@vger.kernel.org]) +AC_INIT([git], + m4_esyscmd([ ./GIT-VERSION-GEN +{ sed -ne 's/GIT_VERSION = //p' GIT-VERSION-FILE | xargs echo -n; } ]), + [git@vger.kernel.org]) I don't think that would warrant dropping the GIT-VERSION-FILE dependency, since the resulting configure script still hard-codes the version number. Yeah, you're right. I was merely sweeping the dependency under the rug :-( diff --git a/Makefile b/Makefile index 736ecd45..2a22041f 100644 --- a/Makefile +++ b/Makefile @@ -2275,7 +2275,7 @@ configure: configure.ac GIT-VERSION-FILE $(RM) $+ ifdef AUTOCONFIGURED -config.status: configure +config.status: configure.ac $(QUIET_GEN)if test -f config.status; then \ ./config.status --recheck; \ else \ The next line just outside the context here does depend on 'configure', which is why I thought this would not be right. But it seems impossible to get away from that, and AUTOCONFIGURED should only be set when ./configure has been run (IIUC), so it's not even realistic to have git reconfigure fail to find ./configure. So, again, looks good. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
diff --git a/Makefile b/Makefile index 26b697d..2f5e2ab 100644 --- a/Makefile +++ b/Makefile @@ -2167,8 +2167,14 @@ configure: configure.ac GIT-VERSION-FILE $(RM) $+ ifdef AUTOCONFIGURED -config.status: configure - $(QUIET_GEN)if test -f config.status; then \ +# We avoid depending on 'configure' here, because it gets rebuilt +# every time GIT-VERSION-FILE is modified, only to update the embedded +# version number string, which config.status does not care about. We +# do want to recheck when the platform/environment detection logic +# changes, hence this depends on configure.ac. +config.status: configure.ac + $(QUIET_GEN)$(MAKE) configure \ + if test -f config.status; then \ ./config.status --recheck; \ else \ ./configure; \ Looks great (at least from my 'make'-incompetent point of view :-)). I do appreciate the comment. Thanks, everyone. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/19] reset: don't allow git reset -- $pathspec in bare repo
--- 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/19] reset $pathspec: exit with code 0 if successful
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More
[PATCH 15/19] reset.c: finish entire cmd_reset() whether or not pathspec is given
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/19] reset.c: extract function for parsing arguments
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 -- To unsubscribe from this list: send the line unsubscribe git in the
[PATCH 19/19] reset [--mixed]: use diff-based reset whether or not pathspec was given
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 . real0m0.219s0m0.080s user0m0.140s0m0.040s sys 0m0.070s0m0.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/19] reset --keep: only write index file once
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 real0m0.315s0m0.296s user0m0.290s0m0.280s sys 0m0.020s0m0.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/19] reset.c: move lock, write and commit out of update_index_refresh()
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/19] reset.c: replace switch by if-else
--- 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/19] reset $pathspec: no need to discard index
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 real0m0.093s0m0.080s user0m0.040s0m0.020s sys 0m0.050s0m0.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/19] reset improvements
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.230.07 reset -q (warm) 0.230.03 reset . (warm) 0.090.07 reset -q . (warm)0.090.03 reset --keep (warm) 0.310.29 reset --keep -q (warm) 0.310.29 reset (cold) 9.742.60 reset -q (cold) 9.850.37 reset . (cold) 2.662.51 reset -q . (cold)2.590.33 reset --keep (cold) 7.587.52 reset --keep -q (cold) 7.377.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/19] reset $sha1 $pathspec: require $sha1 only to be treeish
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
[PATCH 08/19] reset.c: share call to die_if_unmerged_cache()
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/19] reset [--mixed] --quiet: don't refresh index
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 real0m0.215s0m0.176s user0m0.150s0m0.130s sys 0m0.060s0m0.040s And with cold cache (best of five): Before After real0m11.351s 0m8.420s user0m0.230s0m0.220s sys 0m0.270s0m0.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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/19] reset.c: extract function for updating {ORIG,}HEAD
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/19] reset.c: remove unnecessary variable 'i'
Throughout most of parse_args(), the variable 'i' remains at 0. In the remaining few cases, we can do pointer arithmentic on argv itself instead. --- This is clearly mostly a matter of taste. The remainder of the series does not depend on it in any way. builtin/reset.c | 29 ++--- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 9473725..68be05c 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -199,7 +199,6 @@ static void die_if_unmerged_cache(int reset_type) } 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 unused[20]; /* @@ -210,34 +209,34 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c * git reset [-opts] -- paths... * git reset [-opts] paths... * -* At this point, argv[i] points immediately after [-opts]. +* At this point, argv points immediately after [-opts]. */ - if (i argc) { - if (!strcmp(argv[i], --)) { - i++; /* reset to HEAD, possibly with paths */ - } else if (i + 1 argc !strcmp(argv[i+1], --)) { - rev = argv[i]; - i += 2; + if (argc) { + if (!strcmp(argv[0], --)) { + argv++; /* reset to HEAD, possibly with paths */ + } else if (argc 1 !strcmp(argv[1], --)) { + rev = argv[0]; + argv += 2; } /* -* Otherwise, argv[i] could be either rev or paths and +* Otherwise, argv[0] could be either rev or paths and * has to be unambiguous. */ - else if (!get_sha1_committish(argv[i], unused)) { + else if (!get_sha1_committish(argv[0], unused)) { /* -* Ok, argv[i] looks like a rev; it should not +* Ok, argv[0] looks like a rev; it should not * be a filename. */ - verify_non_filename(prefix, argv[i]); - rev = argv[i++]; + verify_non_filename(prefix, argv[0]); + rev = *argv++; } else { /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[i], 1); + verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; - return i argc ? get_pathspec(prefix, argv + i) : NULL; + return *argv ? get_pathspec(prefix, argv) : NULL; } int cmd_reset(int argc, const char **argv, const char *prefix) -- 1.8.1.rc3.331.g1ef2165 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair
We use the path arguments in two places in reset.c: in interactive_reset() and read_from_tree(). Both of these call get_pathspec(), so we pass the (prefix, arv) pair to both functions. Move the call to get_pathspec() out of these methods, for two reasons: 1) One argument is simpler than two. 2) It lets us use the (arguably clearer) if (pathspec) in place of if (i argc). --- If I understand correctly, this should be rebased on top of nd/parse-pathspec. Please let me know. builtin/reset.c | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 65413d0..045c960 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -153,26 +153,15 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int interactive_reset(const char *revision, const char **argv, -const char *prefix) -{ - const char **pathspec = NULL; - - if (*argv) - pathspec = get_pathspec(prefix, argv); - - return run_add_interactive(revision, --patch=reset, pathspec); -} - -static int read_from_tree(const char *prefix, const char **argv, - unsigned char *tree_sha1, int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, + int refresh_flags) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int index_fd; struct diff_options opt; memset(opt, 0, sizeof(opt)); - diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), opt); + diff_tree_setup_paths(pathspec, opt); opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; @@ -216,6 +205,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) 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[] = { @@ -287,22 +277,25 @@ 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})); - return interactive_reset(rev, argv + i, prefix); + return run_add_interactive(rev, --patch=reset, pathspec); } /* git reset tree [--] paths... can be used to * load chosen paths from the tree into the index without * affecting the working tree nor HEAD. */ - if (i argc) { + if (pathspec) { if (reset_type == MIXED) warning(_(--mixed with paths is deprecated; use 'git reset -- paths' instead.)); else if (reset_type != NONE) die(_(Cannot do %s reset with paths.), _(reset_type_names[reset_type])); - return read_from_tree(prefix, argv + i, sha1, + return read_from_tree(pathspec, sha1, quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); } if (reset_type == NONE) -- 1.8.1.rc3.331.g1ef2165 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/19] reset: avoid redundant error message
If writing or committing the new index file fails, we print Could not write new index file. followed by Could not reset index file to revision $rev.. The first message seems to imply the second, so print only the first message. --- builtin/reset.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 8e5d097..54e3c5b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -337,13 +337,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) int err = reset_index(sha1, reset_type, quiet); if (reset_type == KEEP !err) 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); + if (write_cache(newfd, active_cache, active_nr) || + commit_locked_index(lock)) + die(_(Could not write new index file.)); } /* Any resets update HEAD to the head being switched to, -- 1.8.1.rc3.331.g1ef2165 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/19] reset: allow reset on unborn branch
Some users seem to think, knowingly or not, that being on an unborn branch is like having a commit with an empty tree checked out, but when run on an unborn branch, git reset currently fails with: fatal: Failed to resolve 'HEAD' as a valid ref. Instead of making users figure out that they should run git rm --cached -r . , let's teach git reset without a revision argument, when on an unborn branch, to behave as if the user asked to reset to an empty tree. Don't take the analogy with an empty commit too far, though, but still disallow explictly referring to HEAD in git reset HEAD. --- builtin/reset.c| 17 -- t/t7106-reset-unborn-branch.sh | 52 ++ 2 files changed, 62 insertions(+), 7 deletions(-) create mode 100755 t/t7106-reset-unborn-branch.sh diff --git a/builtin/reset.c b/builtin/reset.c index 4c223bd..5cd48ac 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -239,7 +239,7 @@ static int update_refs(const char *rev, const unsigned char *sha1) { 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; + int patch_mode = 0, unborn; const char *rev; unsigned char sha1[20]; const char **pathspec = NULL; @@ -264,8 +264,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH); pathspec = parse_args(argc, argv, prefix, rev); - - if (!pathspec) { + unborn = !strcmp(rev, HEAD) get_sha1(HEAD, sha1); + if (unborn) { + /* reset on unborn branch: treat as reset to empty tree */ + hashcpy(sha1, EMPTY_TREE_SHA1_BIN); + } else if (!pathspec) { if (get_sha1_committish(rev, sha1)) die(_(Failed to resolve '%s' as a valid revision.), rev); commit = lookup_commit_reference(sha1); @@ -285,7 +288,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (patch_mode) { if (reset_type != NONE) die(_(--patch is incompatible with --{hard,mixed,soft})); - return run_add_interactive(rev, --patch=reset, pathspec); + return run_add_interactive(sha1_to_hex(sha1), --patch=reset, pathspec); } /* git reset tree [--] paths... can be used to @@ -337,16 +340,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_(Could not write new index file.)); } - if (!pathspec) { + if (!pathspec !unborn) { /* 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); - - remove_branch_state(); } + if (!pathspec) + remove_branch_state(); return update_ref_status; } diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh new file mode 100755 index 000..4ff6af4 --- /dev/null +++ b/t/t7106-reset-unborn-branch.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='git reset should work on unborn branch' +. ./test-lib.sh + +test_expect_success 'setup' ' + echo a a + echo b b +' + +test_expect_success 'reset' ' + git add a b + git reset + test $(git ls-files) == +' + +test_expect_success 'reset HEAD' ' + rm .git/index + git add a b + test_must_fail git reset HEAD +' + +test_expect_success 'reset $file' ' + rm .git/index + git add a b + git reset a + test $(git ls-files) == b +' + +test_expect_success 'reset -p' ' + rm .git/index + git add a + echo y | git reset -p + test $(git ls-files) == +' + +test_expect_success 'reset --soft is a no-op' ' + rm .git/index + git add a + git reset --soft + test $(git ls-files) == a +' + +test_expect_success 'reset --hard' ' + rm .git/index + git add a + git reset --hard + test $(git ls-files) == + test_path_is_missing a +' + +test_done -- 1.8.1.rc3.331.g1ef2165 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/19] reset.c: move update_index_refresh() call out of read_from_tree()
The final part of cmd_reset() essentially looks like: if (pathspec) { ... read_from_tree(...); } else { ... reset_index(...); update_index_refresh(...); ... } where read_from_tree() internally also calls update_index_refresh(). Move the call to update_index_refresh() out of read_from_tree for symmetry with the 'else' block, making read_from_tree() and reset_index() closer in functionality. --- builtin/reset.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 54e3c5b..a21ba31 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -145,11 +145,8 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, - int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd; struct diff_options opt; memset(opt, 0, sizeof(opt)); @@ -157,7 +154,6 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; - index_fd = hold_locked_index(lock, 1); read_cache(); if (do_diff_cache(tree_sha1, opt)) return 1; @@ -165,7 +161,7 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, diff_flush(opt); diff_tree_release_paths(opt); - return update_index_refresh(index_fd, lock, refresh_flags); + return 0; } static void set_reflog_message(struct strbuf *sb, const char *action, @@ -321,9 +317,13 @@ 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); + 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); + } /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset -- 1.8.1.rc3.331.g1ef2165 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 9, 2013 at 9:01 AM, Jeff King p...@peff.net wrote: On Wed, Jan 09, 2013 at 12:16:13AM -0800, Martin von Zweigbergk wrote: 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. Hmm. But git reset -q git diff-files would not be? Right. Actually, git reset -q git diff was perhaps not a good example, because its analogous plumbing command would be git reset -q git diff-files -p, which is also safe. But, as you say, git reset -q git diff-files (without -p) might list files for which only the stat information has changed. We have never been very clear about which commands refresh the index. Yes, git-reset's documentation doesn't mention it. Since reset is about manipulating the index, I'd expect it to be refreshed afterwards. On the other hand, since we have never guaranteed anything, perhaps a careful script should always use git update-index --refresh. Since git diff-files is a plumbing command, users of it to a hopefully a bit more careful than regular users, but you never know. I would not be too surprised if some of our own scripts are not that careful, though. I didn't find any, but I might have missed something. Regardless, this patch was tangential. The goal of this series can be achieved independently of this patch, so if it's too risky, we can drop easily drop it. Also, even though it does make git reset -q faster, I'm not sure how important that is in practice. Most use cases would probably refresh the index afterwards anyway. In such cases, the improvement on warm cache would still be there, but the relative improvement in the cold cache case would be pretty much gone (since the entire tree would be stat'ed by the following refresh anyway). Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 9, 2013 at 11:12 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: And as a Porcelain, I would rather expect it to leave the resulting index refreshed. Yeah, I guess you're right. Regular users (those using only porcelain) shouldn't notice, but it does make sense to think that the index would be refreshed after running a porcelain. And the risk of breaking people's scripts seems real too. I'll drop patch this from the re-roll (which I'll also make sure I'll sign off) (FYI, the reason I wrote this patch was because I was surprised that git reset did anything with the worktree at all.) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/19] reset: don't allow git reset -- $pathspec in bare repo
On Wed, Jan 9, 2013 at 11:32 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: --- builtin/reset.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) With the patch that does not have any explicit check for bareness nor new error message to scold user with, it is rather hard to tell what is going on, without any description on what (if anything) is broken at the end user level and what remedy is done about that breakage... Will include the following in a re-roll. reset: don't allow git reset -- $pathspec in bare repo Running e.g. git reset . in a bare repo results in an index file being created from the HEAD commit. The differences compared to the index are then printed as usual, but since there is no worktree, it will appear as if all files are deleted. For example, in a bare clone of git.git: Unstaged changes after reset: D .gitattributes D .gitignore D .mailmap ... This happens because the check for is_bare_repository() happens after we branch off into read_from_tree() to reset with paths. Fix by moving the branching point after the check. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/19] reset.c: share call to die_if_unmerged_cache()
On Wed, Jan 9, 2013 at 11:48 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: 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) It is not just equivalent, but should give us identical result, even if we cared the actual value. If err is initially 0, and f() evaluates to 2, err would be 1 in the first case, but 2 in the second case, right? I think the two might be identical in e.g. JavaScript and Python, but I don't use either much. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/21] Add parse_pathspec() that converts cmdline args to struct pathspec
On Sat, Jan 5, 2013 at 10:20 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: + + /* No arguments, no prefix - no pathspec */ + if (!entry !prefix) + return; + /* No arguments with prefix - prefix pathspec */ When working with the old get_pathspec(), I remember wondering if a flag switching between no argument - prefix pathspec and no argument - no pathspec would be worthwhile. I think e.g. 'add' and 'clean' would use the former , while 'reset' and 'commit' would use the latter. Since you're now changing all the callers of get_pathspec(), it seems like the perfect time to ask this question. What do you think? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/19] reset.c: replace switch by if-else
On Wed, Jan 9, 2013 at 11:53 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: --- 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; - } Justification? Clairvoyance -- the HARD case will soon be the only non-empty case. It's also missing KEEP and MERGE (but the empty SOFT block is there). I'll update the message. I will also move the patch a little later in the series, closer to where it will be useful. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/21] commit: convert to use parse_pathspec
On Fri, Jan 11, 2013 at 3:20 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/cache.h b/cache.h index e52365d..a3c316f 100644 --- a/cache.h +++ b/cache.h @@ -476,6 +476,9 @@ extern int ie_modified(const struct index_state *, struct cache_entry *, struct /* Pathspec magic */ #define PATHSPEC_FROMTOP(10) +/* Pathspec flags */ +#define PATHSPEC_EMPTY_MATCH_ALL (10) /* No args means match everything */ + struct pathspec { const char **raw; /* get_pathspec() result, not freed by free_pathspec() */ int nr; diff --git a/setup.c b/setup.c index 6e960b9..a26b6c0 100644 --- a/setup.c +++ b/setup.c @@ -280,6 +280,9 @@ void parse_pathspec(struct pathspec *pathspec, if (!entry !prefix) return; + if (!*argv (flags PATHSPEC_EMPTY_MATCH_ALL)) + return; + /* No arguments with prefix - prefix pathspec */ if (!entry) { static const char *raw[2]; I was surprised not to find these two hunks in 02/21. If they were there, you wouldn't have to explain in the log message of that patch that flags is for future-proofing. Also, *argv is written entry in the surrounding conditions. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/31] Add parse_pathspec() that converts cmdline args to struct pathspec
On Sun, Jan 13, 2013 at 4:35 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: +static void parse_pathspec(struct pathspec *pathspec, + unsigned magic_mask, unsigned flags, + const char *prefix, const char **argv) +{ + struct pathspec_item *item; + const char *entry = *argv; ... + for (i = 0; i n; i++) { + const char *arg = argv[i]; Nit: *argv was assigned to entry above. Reassign that variable instead? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin/reset.c: Fix a sparse warning
On Mon, Jan 14, 2013 at 11:28 AM, Ramsay Jones ram...@ramsay1.demon.co.uk wrote: In particular, sparse issues an symbol not declared. Should it be static? warning for the 'parse_args' function. Since this function does not require greater than file visibility, we suppress this warning by simply adding the static modifier to it's decalaration. Oops, how did I miss that? Will fix in re-roll. Thanks. Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair
We use the path arguments in two places in reset.c: in interactive_reset() and read_from_tree(). Both of these call get_pathspec(), so we pass the (prefix, argv) pair to both functions. Move the call to get_pathspec() out of these methods, for two reasons: 1) One argument is simpler than two. 2) It lets us use the (arguably clearer) if (pathspec) in place of if (i argc). Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 65413d0..045c960 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -153,26 +153,15 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int interactive_reset(const char *revision, const char **argv, -const char *prefix) -{ - const char **pathspec = NULL; - - if (*argv) - pathspec = get_pathspec(prefix, argv); - - return run_add_interactive(revision, --patch=reset, pathspec); -} - -static int read_from_tree(const char *prefix, const char **argv, - unsigned char *tree_sha1, int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, + int refresh_flags) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int index_fd; struct diff_options opt; memset(opt, 0, sizeof(opt)); - diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), opt); + diff_tree_setup_paths(pathspec, opt); opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; @@ -216,6 +205,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) 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[] = { @@ -287,22 +277,25 @@ 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})); - return interactive_reset(rev, argv + i, prefix); + return run_add_interactive(rev, --patch=reset, pathspec); } /* git reset tree [--] paths... can be used to * load chosen paths from the tree into the index without * affecting the working tree nor HEAD. */ - if (i argc) { + if (pathspec) { if (reset_type == MIXED) warning(_(--mixed with paths is deprecated; use 'git reset -- paths' instead.)); else if (reset_type != NONE) die(_(Cannot do %s reset with paths.), _(reset_type_names[reset_type])); - return read_from_tree(prefix, argv + i, sha1, + return read_from_tree(pathspec, sha1, quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); } if (reset_type == NONE) -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/19] reset: don't allow git reset -- $pathspec in bare repo
Running e.g. git reset . in a bare repo results in an index file being created from the HEAD commit. The differences compared to the index are then printed as usual, but since there is no worktree, it will appear as if all files are deleted. For example, in a bare clone of git.git: Unstaged changes after reset: D .gitattributes D .gitignore D .mailmap ... This happens because the check for is_bare_repository() happens after we branch off into read_from_tree() to reset with paths. Fix by moving the branching point after the check. Signed-off-by: Martin von Zweigbergk martinv...@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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/19] reset $pathspec: no need to discard index
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 real0m0.093s0m0.080s user0m0.040s0m0.020s sys 0m0.050s0m0.050s Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- 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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/19] reset $pathspec: exit with code 0 if successful
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. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- 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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 17/19] reset $sha1 $pathspec: require $sha1 only to be treeish
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. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 48 +++- t/t7102-reset.sh | 8 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 520c1a5..b776867 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -178,9 +178,10 @@ static const char **parse_args(const char **argv, const char *prefix, const char /* * 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]. @@ -195,11 +196,13 @@ static const char **parse_args(const char **argv, const char *prefix, const char } /* * 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 ((!argv[1] !get_sha1_committish(argv[0], unused)) || +(argv[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]); @@ -241,7 +244,6 @@ 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; const struct option options[] = { OPT__QUIET(quiet, N_(be quiet, only report errors)), OPT_SET_INT(0, mixed, reset_type, @@ -263,19 +265,23 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH); pathspec = parse_args(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) { + struct commit *commit; + 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) @@ -340,7 +346,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) update_ref_status = update_refs(rev, sha1); if (reset_type == HARD !update_ref_status !quiet) - print_new_head_line(commit); + print_new_head_line(lookup_commit_reference(sha1)); remove_branch_state(); } 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
[PATCH v2 19/19] reset [--mixed]: use diff-based reset whether or not pathspec was given
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 . real0m0.219s0m0.080s user0m0.140s0m0.040s sys 0m0.070s0m0.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). Timing git reset shows that it indeed becomes as fast as git reset . after this patch. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- It seems like a better solution would be for unpack_trees() learn the same tricks as do_diff_cache(). I'm leaving that a challange for the reader :-). I did have a look a unpack_trees(), but it looked rather overwhelming. builtin/reset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 45b01eb..921afbe 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -322,7 +322,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 18/19] reset: allow reset on unborn branch
Some users seem to think, knowingly or not, that being on an unborn branch is like having a commit with an empty tree checked out, but when run on an unborn branch, git reset currently fails with: fatal: Failed to resolve 'HEAD' as a valid ref. Instead of making users figure out that they should run git rm --cached -r . , let's teach git reset without a revision argument, when on an unborn branch, to behave as if the user asked to reset to an empty tree. Don't take the analogy with an empty commit too far, though, but still disallow explictly referring to HEAD in git reset HEAD. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c| 16 - t/t7106-reset-unborn-branch.sh | 52 ++ 2 files changed, 62 insertions(+), 6 deletions(-) create mode 100755 t/t7106-reset-unborn-branch.sh diff --git a/builtin/reset.c b/builtin/reset.c index b776867..45b01eb 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -240,7 +240,7 @@ static int update_refs(const char *rev, const unsigned char *sha1) 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; + int patch_mode = 0, unborn; const char *rev; unsigned char sha1[20]; const char **pathspec = NULL; @@ -265,7 +265,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH); pathspec = parse_args(argv, prefix, rev); - if (!pathspec) { + unborn = !strcmp(rev, HEAD) get_sha1(HEAD, sha1); + if (unborn) { + /* reset on unborn branch: treat as reset to empty tree */ + hashcpy(sha1, EMPTY_TREE_SHA1_BIN); + } else if (!pathspec) { struct commit *commit; if (get_sha1_committish(rev, sha1)) die(_(Failed to resolve '%s' as a valid revision.), rev); @@ -286,7 +290,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (patch_mode) { if (reset_type != NONE) die(_(--patch is incompatible with --{hard,mixed,soft})); - return run_add_interactive(rev, --patch=reset, pathspec); + return run_add_interactive(sha1_to_hex(sha1), --patch=reset, pathspec); } /* git reset tree [--] paths... can be used to @@ -340,16 +344,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_(Could not write new index file.)); } - if (!pathspec) { + if (!pathspec !unborn) { /* 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(lookup_commit_reference(sha1)); - - remove_branch_state(); } + if (!pathspec) + remove_branch_state(); return update_ref_status; } diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh new file mode 100755 index 000..8062cf5 --- /dev/null +++ b/t/t7106-reset-unborn-branch.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='git reset should work on unborn branch' +. ./test-lib.sh + +test_expect_success 'setup' ' + echo a a + echo b b +' + +test_expect_success 'reset' ' + git add a b + git reset + test $(git ls-files) = +' + +test_expect_success 'reset HEAD' ' + rm .git/index + git add a b + test_must_fail git reset HEAD +' + +test_expect_success 'reset $file' ' + rm .git/index + git add a b + git reset a + test $(git ls-files) = b +' + +test_expect_success 'reset -p' ' + rm .git/index + git add a + echo y | git reset -p + test $(git ls-files) = +' + +test_expect_success 'reset --soft is a no-op' ' + rm .git/index + git add a + git reset --soft + test $(git ls-files) = a +' + +test_expect_success 'reset --hard' ' + rm .git/index + git add a + git reset --hard + test $(git ls-files) = + test_path_is_missing a +' + +test_done -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 15/19] reset.c: finish entire cmd_reset() whether or not pathspec is given
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(). Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 42 ++ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index e8a3e41..c316d9b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -309,19 +309,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. */ @@ -331,11 +318,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( @@ -346,14 +338,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/19] reset.c: move lock, write and commit out of update_index_refresh()
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.). Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 34 ++ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 70733c2..c1d6ef2 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, @@ -321,9 +312,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 @@ -351,9 +347,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/19] reset: avoid redundant error message
If writing or committing the new index file fails, we print Could not write new index file. followed by Could not reset index file to revision $rev.. The first message seems to imply the second, so print only the first message. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 7c440ad..97fa9f7 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -338,13 +338,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) int err = reset_index(sha1, reset_type, quiet); if (reset_type == KEEP !err) 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); + if (write_cache(newfd, active_cache, active_nr) || + commit_locked_index(lock)) + die(_(Could not write new index file.)); } /* Any resets update HEAD to the head being switched to, -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 14/19] reset [--mixed]: only write index file once
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 real0m0.239s0m0.214s user0m0.160s0m0.130s sys 0m0.070s0m0.080s Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c1d6ef2..e8a3e41 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -336,6 +336,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.)); @@ -347,15 +352,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/19] reset.c: extract function for parsing arguments
Declutter cmd_reset() a bit by moving out the argument parsing to its own function. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 70 +++-- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 664fad9..58f0f61 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -198,36 +198,11 @@ static void die_if_unmerged_cache(int reset_type) } -int cmd_reset(int argc, const char **argv, const char *prefix) +static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) { - int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0; - int patch_mode = 0; + 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 +225,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 +237,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 +286,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.1.454.gce43f05
[PATCH v2 11/19] reset.c: replace switch by if-else
The switch statement towards the end of reset.c is missing case arms for KEEP and MERGE for no obvious reason, and soon the only non-empty case arm will be the one for HARD. So let's proactively replace it by if-else, which will let us move one if statement out without leaving funny-looking left-overs. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 13 +++-- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 97fa9f7..c3eb2eb 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -349,18 +349,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 16/19] reset.c: inline update_index_refresh()
Now that there is only one caller left to the single-line method update_index_refresh(), inline it. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c316d9b..520c1a5 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) { @@ -329,9 +323,11 @@ 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) { /* Report what has not been updated. */ + int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; + refresh_index(the_index, flags, NULL, NULL, + _(Unstaged changes after reset:)); + } if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(lock)) -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/19] reset.c: extract function for updating {ORIG_,}HEAD
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. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 39 +++ 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index d89cf4d..2187d64 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -240,16 +240,35 @@ static const char **parse_args(const char **argv, const char *prefix, const char return argv[0] ? 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, @@ -333,17 +352,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: @@ -360,7 +369,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/19] reset --keep: only write index file once
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 real0m0.315s0m0.296s user0m0.290s0m0.280s sys 0m0.020s0m0.010s Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4e34195..7c440ad 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; } @@ -341,9 +333,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/19] reset improvements
Changes since v1: - Spelling fixes. - Explained how git reset -- $pathspec in bare repo is broken. - Provided motivation for replacement of switch by if-else - Fixed argv/argc handling by removing use of argc. - Replaced don't refresh index on --quiet patch by one that just inlines update_index_refresh() - Incorporated fixes from Junio's repo - Provided some motivation for replace switch by if-else amd moved the patch later in the series. Thanks for reviewing! 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 --keep: only write index file once reset: avoid redundant error message reset.c: replace switch by if-else 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]: only write index file once reset.c: finish entire cmd_reset() whether or not pathspec is given reset.c: inline update_index_refresh() 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| 283 +++-- t/t2013-checkout-submodule.sh | 2 +- t/t7102-reset.sh | 26 +++- t/t7106-reset-unborn-branch.sh | 52 4 files changed, 203 insertions(+), 160 deletions(-) create mode 100755 t/t7106-reset-unborn-branch.sh -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/19] reset.c: move update_index_refresh() call out of read_from_tree()
The final part of cmd_reset() essentially looks like: if (pathspec) { ... read_from_tree(...); } else { ... reset_index(...); update_index_refresh(...); ... } where read_from_tree() internally also calls update_index_refresh(). Move the call to update_index_refresh() out of read_from_tree for symmetry with the 'else' block, making read_from_tree() and reset_index() closer in functionality. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c3eb2eb..70733c2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -145,11 +145,8 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, - int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd; struct diff_options opt; memset(opt, 0, sizeof(opt)); @@ -157,7 +154,6 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; - index_fd = hold_locked_index(lock, 1); read_cache(); if (do_diff_cache(tree_sha1, opt)) return 1; @@ -165,7 +161,7 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, diff_flush(opt); diff_tree_release_paths(opt); - return update_index_refresh(index_fd, lock, refresh_flags); + return 0; } static void set_reflog_message(struct strbuf *sb, const char *action, @@ -322,9 +318,13 @@ 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); + 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); + } /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 08/19] reset.c: share call to die_if_unmerged_cache()
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) Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- builtin/reset.c | 14 ++ 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 2187d64..4e34195 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -337,15 +337,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.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/19] reset.c: remove unnecessary variable 'i'
Throughout most of parse_args(), the variable 'i' remains at 0. Many references are still made to the variable even when it could only have the value 0. This made at least me, who has relatively little experience with C programming styles, think that parts of the function was meant to be part of a loop. To avoid such confusion, remove the variable and also the 'argc' parameter and check for NULL trailing argv instead. Signed-off-by: Martin von Zweigbergk martinv...@gmail.com --- I explained a bit more why I was confused by the current style, but I'm also perfectly happy if you just drop the patch (there would be some minor conflicts in a later patch, though). builtin/reset.c | 33 - 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 58f0f61..d89cf4d 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -198,9 +198,8 @@ static void die_if_unmerged_cache(int reset_type) } -static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) +static const char **parse_args(const char **argv, const char *prefix, const char **rev_ret) { - int i = 0; const char *rev = HEAD; unsigned char unused[20]; /* @@ -211,34 +210,34 @@ static const char **parse_args(int argc, const char **argv, const char *prefix, * git reset [-opts] -- paths... * git reset [-opts] paths... * -* At this point, argv[i] points immediately after [-opts]. +* At this point, argv points immediately after [-opts]. */ - if (i argc) { - if (!strcmp(argv[i], --)) { - i++; /* reset to HEAD, possibly with paths */ - } else if (i + 1 argc !strcmp(argv[i+1], --)) { - rev = argv[i]; - i += 2; + if (argv[0]) { + if (!strcmp(argv[0], --)) { + argv++; /* reset to HEAD, possibly with paths */ + } else if (argv[1] !strcmp(argv[1], --)) { + rev = argv[0]; + argv += 2; } /* -* Otherwise, argv[i] could be either rev or paths and +* Otherwise, argv[0] could be either rev or paths and * has to be unambiguous. */ - else if (!get_sha1_committish(argv[i], unused)) { + else if (!get_sha1_committish(argv[0], unused)) { /* -* Ok, argv[i] looks like a rev; it should not +* Ok, argv[0] looks like a rev; it should not * be a filename. */ - verify_non_filename(prefix, argv[i]); - rev = argv[i++]; + verify_non_filename(prefix, argv[0]); + rev = *argv++; } else { /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[i], 1); + verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; - return i argc ? get_pathspec(prefix, argv + i) : NULL; + return argv[0] ? get_pathspec(prefix, argv) : NULL; } int cmd_reset(int argc, const char **argv, const char *prefix) @@ -270,7 +269,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH); - pathspec = parse_args(argc, argv, prefix, rev); + pathspec = parse_args(argv, prefix, rev); if (get_sha1_committish(rev, sha1)) die(_(Failed to resolve '%s' as a valid ref.), rev); -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/19] reset.c: remove unnecessary variable 'i'
I suppose this was meant for everyone. Adding back the others. On Tue, Jan 15, 2013 at 10:27 AM, Holding, Lawrence (NZ) lawrence.hold...@cubic.com wrote: Maybe use *argv instead of argv[0]? Sure. Everywhere? Also in the lines added in patch 17/19 that refer to both argv[0] and argv[1], such as argv[1] !get_sha1_treeish(argv[0], unused)? Or is this just a sign that I'm making the code _more_ confusing to those who are more familiar with C? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 17/19] fixup! reset $sha1 $pathspec: require $sha1 only to be treeish
--- Sorry, I forgot the documentation updates. I hope this looks ok. Can you squash this in, Junio? Thanks. I don't think any documentation update is necessary for the reset on unborn branch patch. Let me know if you think differently. Documentation/git-reset.txt | 18 +- builtin/reset.c | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 978d8da..a404b47 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -8,20 +8,20 @@ git-reset - Reset current HEAD to the specified state SYNOPSIS [verse] -'git reset' [-q] [commit] [--] paths... -'git reset' (--patch | -p) [commit] [--] [paths...] +'git reset' [-q] [tree-ish] [--] paths... +'git reset' (--patch | -p) [tree-sh] [--] [paths...] 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [commit] DESCRIPTION --- -In the first and second form, copy entries from commit to the index. +In the first and second form, copy entries from tree-ish to the index. In the third form, set the current branch head (HEAD) to commit, optionally -modifying index and working tree to match. The commit defaults to HEAD -in all forms. +modifying index and working tree to match. The tree-ish/commit defaults +to HEAD in all forms. -'git reset' [-q] [commit] [--] paths...:: +'git reset' [-q] [tree-ish] [--] paths...:: This form resets the index entries for all paths to their - state at commit. (It does not affect the working tree, nor + state at tree-ish. (It does not affect the working tree, nor the current branch.) + This means that `git reset paths` is the opposite of `git add @@ -34,9 +34,9 @@ Alternatively, using linkgit:git-checkout[1] and specifying a commit, you can copy the contents of a path out of a commit to the index and to the working tree in one go. -'git reset' (--patch | -p) [commit] [--] [paths...]:: +'git reset' (--patch | -p) [tree-ish] [--] [paths...]:: Interactively select hunks in the difference between the index - and commit (defaults to HEAD). The chosen hunks are applied + and tree-ish (defaults to HEAD). The chosen hunks are applied in reverse to the index. + This means that `git reset -p` is the opposite of `git add -p`, i.e. diff --git a/builtin/reset.c b/builtin/reset.c index b776867..cb84f1b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -23,8 +23,8 @@ static const char * const git_reset_usage[] = { N_(git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [commit]), - N_(git reset [-q] commit [--] paths...), - N_(git reset --patch [commit] [--] [paths...]), + N_(git reset [-q] tree-ish [--] paths...), + N_(git reset --patch [tree-ish] [--] [paths...]), NULL }; -- 1.8.1.1.454.gce43f05 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/19] fixup! reset $sha1 $pathspec: require $sha1 only to be treeish
On Wed, Jan 16, 2013 at 10:00 AM, Martin von Zweigbergk martinv...@gmail.com wrote: --- Sorry, I forgot the documentation updates. I hope this looks ok. Can you squash this in, Junio? Thanks. I see the series just entered 'next', so I guess it would have to go on top then. Perhaps with a commit message like as simple as the following. Let me know if you prefer it to be resent as a proper patch. Sorry about the noise. reset: update documentation to require only tree-ish with paths When resetting with paths, we no longer require a commit argument, but only a tree-ish. Update the documentation and synopsis accordingly. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] git rm -u
On Sun, Jan 20, 2013 at 1:27 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: git add -u is one of the only exceptions (with git grep). I consider this as a bug, and think this should be changed. This has been discussed several times here, but no one took the time to actually do the change - As we have the from the root magic pathspec these days, requiring git add -u :/ when the user really means to add everything is no longer too much of a burden, but if we suddenly changed git add -u to mean git add -u ., that is too much of a change in the semantics. And I think someone (Jeff?) pointed out that that last part is even more true for git clean, which also currently works on the current directory if not told otherwise. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/31] parse_pathspec: add PATHSPEC_EMPTY_MATCH_ALL
Hi, I was tempted to ask this before, and the recent thread regarding add -u/A [1] convinced me to. On Sun, Jan 13, 2013 at 4:35 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: We have two ways of dealing with empty pathspec: 1. limit it to current prefix 2. match the entire working directory Some commands go with #1, some with #2. get_pathspec() and parse_pathspec() only supports #1. Make it support #2 too via PATHSPEC_EMPTY_MATCH_ALL flag. If #2 is indeed the direction we want to go, then maybe we should make that the default behavior from parse_pathspec()? I.e. rename the flag PATHSPEC_EMPTY_MATCH_PREFIX (or something). Makes sense? Btw, Matthieu was asking where we use #1. If you do invert the name and meaning of the flag, then the answer to that question should be (mostly?) obvious from a re-roll of your series (i.e. all the places where PATHSPEC_EMPTY_MATCH_PREFIX is used). Martin [1] http://thread.gmane.org/gmane.comp.version-control.git/213988/focus=214113 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] git-am: record full index line in the patch used while rebasing
On Thu, Jan 31, 2013 at 8:32 PM, Junio C Hamano gits...@pobox.com wrote: Earlier, a230949 (am --rebasing: get patch body from commit, not from mailbox, 2012-06-26) learned to regenerate patch body from the commit object while rebasing, instead of reading from the rebase-am front-end. While doing so, it used git diff-tree but without giving it the --full-index option. This does not matter for in-repository objects; during rebasing, any abbreviated object name should uniquely identify them. But we may be rebasing a commit that contains a change to a gitlink, in which case we usually should not have the object (it names a commit in the submodule). A full object name is necessary to later reconstruct a fake ancestor index for them. From what I can understand, this all makes sense. I didn't notice the emails about the breakage until now, but I wouldn't have known where to even start looking anyway, so thanks a lot for taking care of it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase --preserve-merges keeps empty merge commits
I'm working on a re-roll of http://thread.gmane.org/gmane.comp.version-control.git/205796 and finally got around to including test cases for what you fixed in this patch. I want to make sure I'm testing what you fixed here. See questions below. On Sat, Jan 12, 2013 at 12:46 PM, Phil Hord ho...@cisco.com wrote: Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20) 'git rebase --preserve-merges' fails to preserve empty merge commits unless --keep-empty is also specified. Merge commits should be preserved in order to preserve the structure of the rebased graph, even if the merge commit does not introduce changes to the parent. Teach rebase not to drop merge commits only because they are empty. Consider a history like # a---b---c # \ \ # d---l #\ # e # \ # C where 'l' is tree-same with 'd' and 'C' introduces the same change as 'c'. My test case runs 'git rebase -p e l' and expects the result to look like # a---b---c # \ \ # d \ #\ \ # e---l A special case which is not handled by this change is for a merge commit whose parents are now the same commit because all the previous different parents have been dropped as a result of this rebase or some previous operation. And for this case, the test case runs 'git rebase -p C l'. Is that what you meant here? Before your patch, git would just say Nothing to do and after your patch, we get # a---b---c # \ \ # d \ #\ \ # e \ # \ \ # C---l As you say, your patch doesn't try to handle this case, but at least the new behavior seems better. I think we would ideally want the recreated 'l' to have only 'C' as parent in this case. Does that make sense? Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase --preserve-merges keeps empty merge commits
On Fri, Feb 1, 2013 at 1:05 PM, Phil Hord ho...@cisco.com wrote: This is probably right, but it is not exactly the case that caused my itch. I think my branch looked like [...] That also makes sense. I'll add tests for both cases. Your patch makes both of them pass. # a---b---c # \ \ # d \ #\ \ # e \ # \ \ # C---l As you say, your patch doesn't try to handle this case, but at least the new behavior seems better. I think we would ideally want the recreated 'l' to have only 'C' as parent in this case. Does that make sense? This is not what I meant, but it is a very interesting corner case. I am not sure I have a solid opinion on what the result should be here. Neither do I, so I'll just drop the test case. Thanks. Here is the corner case I was thinking of. I did not test this to see if this will happen, but I conceived that it might. Suppose you have this tree where # a---b---c # \ # d---g---l #\ / # C where 'C' introduced the same changes as 'c'. When I execute 'git rebase -p l c', I expect that I will end up with this: # a---b---c---d--- # \ \ # ---g---l That is, 'C' gets skipped because it introduces the same changes already seen in 'c'. So 'g' now has two parents: 'd' and 'C^'. But 'C^' is 'd', so 'g' now has two parents, both of whom are 'd'. I think it should collapse to this instead: # a---b---c---d---g---l I think this is actually what you will get. But I think it will only be linearized if the branch that should be dropped is the second parent. I have two tests for this, but I need to simplify them a little to see that that (parent number) is the only difference. I hope this is clear, but please let me know if I made it too confusing. Very clear. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: `git checkout --orpan` leaves a dirty worktree
I'm curious what your use case is. The behavior has been inconvenient for me too, but I have only used it in test cases; I have no real use case where I wanted to create an unborn/orphan branch. On Fri, Feb 8, 2013 at 11:50 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Hi, Why should I have to `git rm -rf .` after a `git checkout --orphan`? What sort of misfeature/ incomplete feature is this? Ram -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 4/4] git-rebase: add keep_empty flag
On Fri, Apr 20, 2012 at 7:36 AM, Neil Horman nhor...@tuxdriver.com wrote: pick_one () { ff=--ff + case $1 in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac case $force_rebase in '') ;; ?*) ff= ;; esac output git rev-parse --verify $sha1 || die Invalid commit name: $sha1 + + if is_empty_commit $sha1 + then + empty_args=--allow-empty + fi + test -d $rewritten pick_one_preserving_merges $@ return - output git cherry-pick $ff $@ + output git cherry-pick $empty_args $ff $@ The is_empty_commit check seems to mean that if $sha1 is an empty commit, we pass the --allow-empty option to cherry-pick. If it's not empty, we don't. The word allow in allow-empty suggests that even if the commit is not empty, cherry-pick would not mind. So, can we always pass allow-empty to cherry-pick (i.e. even if the commit to pick is not empty)? Sorry I'm commenting so late; I didn't have time to look at your patches when you sent them, but I'm currently working on the code touched by this patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 4/4] git-rebase: add keep_empty flag
On Wed, Jul 18, 2012 at 12:10 AM, Johannes Sixt j.s...@viscovery.net wrote: Am 7/18/2012 8:20, schrieb Martin von Zweigbergk: On Fri, Apr 20, 2012 at 7:36 AM, Neil Horman nhor...@tuxdriver.com wrote: pick_one () { ff=--ff + case $1 in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac case $force_rebase in '') ;; ?*) ff= ;; esac output git rev-parse --verify $sha1 || die Invalid commit name: $sha1 + + if is_empty_commit $sha1 + then + empty_args=--allow-empty + fi + test -d $rewritten pick_one_preserving_merges $@ return - output git cherry-pick $ff $@ + output git cherry-pick $empty_args $ff $@ The is_empty_commit check seems to mean that if $sha1 is an empty commit, we pass the --allow-empty option to cherry-pick. If it's not empty, we don't. The word allow in allow-empty suggests that even if the commit is not empty, cherry-pick would not mind. So, can we always pass allow-empty to cherry-pick (i.e. even if the commit to pick is not empty)? I don't think so. If the commit is not empty, but all its changes are already in HEAD, then it will become empty when cherry-picked to HEAD. In such a case, we usually do not want to record an empty commit, but stop rebase to give to user a chance to deal with the situation. Ah, makes sense. Thanks! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/7] git-rebase--interactive: group all $preserve_merges code
The code in git-rebase--interactive that creates the todo file contains if-blocks that depend on whether $preserve_merges is active. There is only a very small amount of code in between that is shared with non-merge-preserving code path, so remove the repeated conditions and duplicate the small amount of shared code instead. --- git-rebase--interactive.sh | 69 ++ 1 file changed, 33 insertions(+), 36 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index fa722b6..4bb8e3f 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -793,28 +793,6 @@ mkdir $state_dir || die Could not create temporary $state_dir : $state_dir/interactive || die Could not mark as interactive write_basic_state -if test t = $preserve_merges -then - if test -z $rebase_root - then - mkdir $rewritten - for c in $(git merge-base --all $orig_head $upstream) - do - echo $onto $rewritten/$c || - die Could not init rewritten commits - done - else - mkdir $rewritten - echo $onto $rewritten/root || - die Could not init rewritten commits - fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= -else - merges_option=--no-merges --cherry-pick -fi shorthead=$(git rev-parse --short $orig_head) shortonto=$(git rev-parse --short $onto) @@ -839,16 +817,30 @@ add_pick_line () { printf '%s\n' ${comment_out}pick $1 $2 $todo } -git rev-list $merges_option --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --topo-order \ - $revisions | \ - sed -n s/^//p | -while read -r shortsha1 rest -do - if test t != $preserve_merges +if test t = $preserve_merges +then + if test -z $rebase_root then - add_pick_line $shortsha1 $rest + mkdir $rewritten + for c in $(git merge-base --all $orig_head $upstream) + do + echo $onto $rewritten/$c || + die Could not init rewritten commits + done else + mkdir $rewritten + echo $onto $rewritten/root || + die Could not init rewritten commits + fi + # No cherry-pick because our first pass is to determine + # parents to rewrite and skipping dropped commits would + # prematurely end our probe + git rev-list --pretty=oneline --abbrev-commit \ + --abbrev=7 --reverse --left-right --topo-order \ + $revisions | + sed -n s/^//p | + while read -r shortsha1 rest + do sha1=$(git rev-parse $shortsha1) if test -z $rebase_root then @@ -868,12 +860,8 @@ do touch $rewritten/$sha1 add_pick_line $shortsha1 $rest fi - fi -done - -# Watch for commits that been dropped by --cherry-pick -if test t = $preserve_merges -then + done + # Watch for commits that been dropped by --cherry-pick mkdir $dropped # Save all non-cherry-picked changes git rev-list $revisions --left-right --cherry-pick | \ @@ -895,6 +883,15 @@ then rm $rewritten/$rev fi done +else + git rev-list --no-merges --cherry-pick --pretty=oneline --abbrev-commit \ + --abbrev=7 --reverse --left-right --topo-order \ + $revisions | + sed -n s/^//p | + while read -r shortsha1 rest + do + add_pick_line $shortsha1 $rest + done fi test -s $todo || echo noop $todo -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] git-rebase--interactive.sh: extract function for adding pick line
Extract the code that adds a possibly commented-out pick line to the todo file. This lets us reuse it more easily later. --- git-rebase--interactive.sh | 21 - 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index bef7bc0..fa722b6 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -828,23 +828,26 @@ else revisions=$onto...$orig_head shortrevisions=$shorthead fi -git rev-list $merges_option --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --topo-order \ - $revisions | \ - sed -n s/^//p | -while read -r shortsha1 rest -do - if test -z $keep_empty is_empty_commit $shortsha1 +add_pick_line () { + if test -z $keep_empty is_empty_commit $1 then comment_out=# else comment_out= fi + printf '%s\n' ${comment_out}pick $1 $2 $todo +} +git rev-list $merges_option --pretty=oneline --abbrev-commit \ + --abbrev=7 --reverse --left-right --topo-order \ + $revisions | \ + sed -n s/^//p | +while read -r shortsha1 rest +do if test t != $preserve_merges then - printf '%s\n' ${comment_out}pick $shortsha1 $rest $todo + add_pick_line $shortsha1 $rest else sha1=$(git rev-parse $shortsha1) if test -z $rebase_root @@ -863,7 +866,7 @@ do if test f = $preserve then touch $rewritten/$sha1 - printf '%s\n' ${comment_out}pick $shortsha1 $rest $todo + add_pick_line $shortsha1 $rest fi fi done -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] rebase -p: use --cherry-mark for todo file
While building the todo file, 'rebase -p' needs to find the cherry-picked commits in the branch that is about to be rebased. For this, it calculates the set difference between the full set of commits and the non-cherry-picked ones (as reported by 'git rev-list --left-right --cherry-pick'). Now that have the 'git rev-list --cherry-mark' option (since adbbb31 (revision.c: introduce --cherry-mark, 2011-03-07)), we can instead use that option to get the set of cherry-picked commits. --- git-rebase--interactive.sh | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 9715830..47beb58 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -859,17 +859,12 @@ then add_pick_line $sha1 fi done - # Watch for commits that been dropped by --cherry-pick + # Now drop cherry-picked commits mkdir $dropped - # Save all non-cherry-picked changes - git rev-list $revisions --left-right --cherry-pick | \ - sed -n s/^//p $state_dir/not-cherry-picks - # Now all commits and note which ones are missing in - # not-cherry-picks and hence being dropped - git rev-list $revisions | + git rev-list $revisions --cherry-mark --right-only | sed -ne s/^=//p | while read rev do - if test -f $rewritten/$rev -a $(sane_grep $rev $state_dir/not-cherry-picks) = + if test -f $rewritten/$rev then # Use -f2 because if rev-list is telling us this commit is # not worthwhile, we don't want to track its multiple heads, -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] rebase -p: don't request --left-right only to ignore left side
While generating the todo file, rebase -p calls 'git rev-list --left-right a...b' (with 'a' equal to $upstream or $onto and 'b' equal to $orig_head) and its output is piped through 'sed -n s/^//p', making it equivalent to 'git rev-list --right-only a...b'. Change the invocation to exactly that. (One could alternatively change it to 'git rev-list a..b', which would be even simpler, if it wasn't for the fact that we already have the revision range expression in a variable.) --- git-rebase--interactive.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 47beb58..cd5a2cc 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -836,8 +836,7 @@ then # No cherry-pick because our first pass is to determine # parents to rewrite and skipping dropped commits would # prematurely end our probe - git rev-list $revisions --reverse --left-right --topo-order | - sed -n s/^//p | + git rev-list $revisions --reverse --right-only --topo-order | while read -r sha1 do if test -z $rebase_root -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] git-rebase--interactive.sh: look up subject in add_pick_line
The todo file is generated using (more-or-less) 'git rev-list $revisions --pretty=oneline --abbrev-commit --abbrev=7', i.e. by letting 'git rev-list' output both the abbreviated sha1 and the subject line. To allow us to more easily generate the list of commits to rebase by using commands that don't support outputting the subject line, move this logic into add_pick_line. --- git-rebase--interactive.sh | 21 + 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 4bb8e3f..9715830 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -814,7 +814,8 @@ add_pick_line () { else comment_out= fi - printf '%s\n' ${comment_out}pick $1 $2 $todo + line=$(git rev-list -1 --pretty=oneline --abbrev-commit --abbrev=7 $1) + printf '%s\n' ${comment_out}pick $line $todo } if test t = $preserve_merges @@ -835,13 +836,10 @@ then # No cherry-pick because our first pass is to determine # parents to rewrite and skipping dropped commits would # prematurely end our probe - git rev-list --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --topo-order \ - $revisions | + git rev-list $revisions --reverse --left-right --topo-order | sed -n s/^//p | - while read -r shortsha1 rest + while read -r sha1 do - sha1=$(git rev-parse $shortsha1) if test -z $rebase_root then preserve=t @@ -858,7 +856,7 @@ then if test f = $preserve then touch $rewritten/$sha1 - add_pick_line $shortsha1 $rest + add_pick_line $sha1 fi done # Watch for commits that been dropped by --cherry-pick @@ -884,13 +882,12 @@ then fi done else - git rev-list --no-merges --cherry-pick --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --topo-order \ - $revisions | + git rev-list $revisions --reverse --left-right --topo-order \ + --no-merges --cherry-pick | sed -n s/^//p | - while read -r shortsha1 rest + while read -r sha1 do - add_pick_line $shortsha1 $rest + add_pick_line $sha1 done fi -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] correctly calculate patches to rebase
These seven patches replace the broken one I sent in http://thread.gmane.org/gmane.comp.version-control.git/200644/focus=200648. I hope I got the handling of empty commits right this time. Martin von Zweigbergk (7): git-rebase--am.sh: avoid special-casing --keep-empty git-rebase--interactive.sh: extract function for adding pick line git-rebase--interactive: group all $preserve_merges code git-rebase--interactive.sh: look up subject in add_pick_line rebase -p: use --cherry-mark for todo file rebase -p: don't request --left-right only to ignore left side rebase (without -p): correctly calculate patches to rebase git-am.sh | 10 +- git-rebase--am.sh | 20 +++ git-rebase--interactive.sh | 87 -- git-rebase--merge.sh | 2 +- git-rebase.sh | 11 +++--- t/t3401-rebase-partial.sh | 17 + t/t3406-rebase-message.sh | 14 7 files changed, 81 insertions(+), 80 deletions(-) -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] rebase (without -p): correctly calculate patches to rebase
The different types of rebase use different ways of calculating the patches to rebase. 'git rebase' (without -m/-i/-p) uses git format-patch --ignore-if-in-upstream $upstream..$orig_head 'git rebase -m' uses git rev-list $upstream..$orig_head 'git rebase -i' (without -p) uses git rev-list $upstream...$orig_head --left-right --no-merges \ --cherry-pick | sed -n s/^//p , which could also have been written git rev-list $upstream...$orig_head --right-only --no-merges \ --cherry-pick 'git rebase -p' uses git rev-list $upstream...$orig_head --right-only followed by cherry-picked commits found by git rev-list $upstream...$orig_head --cherry-mark --right-only | | sed -ne s/^=//p As Knut Franke reported in [1], the fact that there is no --ignore-if-in-upstream or equivalent when using merge-based rebase means that unnecessary conflicts can arise due to commits cherry-picked between $orig_head and $upstream. With all the other types, there is a different problem with the method of calculating the commits to rebase. Copying the example history from [1]: .-c / a---b---d---e---f \ .-g---E Commit E is here a cherry-pick of e. If we now run 'git rebase [-i|-p] --onto c f E', the commits to rebase will be those on E that are not equivalent to any of those in f, which in this case would be only 'g'. Commit 'E' would thus be lost. To solve both of the above problems, we want to find the commits in $upstream..$orig_head that are not cherry-picked in $orig_head..$onto. There is unfortunately no direct way of finding these commits using 'git rev-list', so we will have to resort to using 'git cherry' and filter for lines starting with '+'. This works for all but 'rebase -p', since 'git cherry' ignores merges. As a side-effect, we also avoid the cost of formatting patches. Test case updates for 'rebase -m' by Knut, the rest by Martin. [1] http://thread.gmane.org/gmane.comp.version-control.git/161917 Helped-by: Knut Franke knut.fra...@gmx.de Signed-off-by: Martin von Zweigbergk martin.von.zweigbe...@gmail.com --- git-rebase--am.sh | 6 ++ git-rebase--interactive.sh | 8 +++- git-rebase--merge.sh | 2 +- git-rebase.sh | 11 --- t/t3401-rebase-partial.sh | 17 + t/t3406-rebase-message.sh | 14 +++--- 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 37c1b23..fe3fdd1 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -16,11 +16,9 @@ skip) ;; esac -test -n $rebase_root root_flag=--root test -n $keep_empty git_am_opt=$git_am_opt --keep-empty -git format-patch -k --stdout --full-index --ignore-if-in-upstream \ - --src-prefix=a/ --dst-prefix=b/ \ - --no-renames $root_flag $revisions | +generate_revisions | +sed -e 's/\([0-9a-f]\{40\}\)/From \1 Mon Sep 17 00:00:00 2001/' | git am $git_am_opt --rebasing --resolvemsg=$resolvemsg move_to_original_branch diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index cd5a2cc..da32ca7 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -800,10 +800,8 @@ if test -z $rebase_root # this is now equivalent to ! -z $upstream then shortupstream=$(git rev-parse --short $upstream) - revisions=$upstream...$orig_head shortrevisions=$shortupstream..$shorthead else - revisions=$onto...$orig_head shortrevisions=$shorthead fi @@ -822,6 +820,7 @@ if test t = $preserve_merges then if test -z $rebase_root then + revisions=$upstream...$orig_head mkdir $rewritten for c in $(git merge-base --all $orig_head $upstream) do @@ -829,6 +828,7 @@ then die Could not init rewritten commits done else + revisions=$onto...$orig_head mkdir $rewritten echo $onto $rewritten/root || die Could not init rewritten commits @@ -876,9 +876,7 @@ then fi done else - git rev-list $revisions --reverse --left-right --topo-order \ - --no-merges --cherry-pick | - sed -n s/^//p | + generate_revisions | while read -r sha1 do add_pick_line $sha1 diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index b10f2cf..bf4ec4b 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -131,7 +131,7 @@ echo $onto_name $state_dir/onto_name write_basic_state msgnum=0 -for cmt in `git rev-list --reverse --no-merges $revisions` +for cmt in $(generate_revisions) do msgnum=$(($msgnum + 1)) echo $cmt $state_dir/cmt.$msgnum diff --git a/git-rebase.sh b/git-rebase.sh index 1cd0633..0fdff87 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -530,6 +530,10 @@ then GIT_PAGER='' git diff --stat
Re: [PATCH 0/7] correctly calculate patches to rebase
Argh! Sorry about the sendemail.chainreplyto=true. I must have read that warning message incorrectly :-(. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] git-rebase--am.sh: avoid special-casing --keep-empty
Since 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26), the patch body to apply when running 'git am --rebasing' is not taken from the mbox, but directly from the commit. If such a commit is empty, 'git am --rebasing' still happily applies it and commits. However, since the input to 'git am --rebasing' only ever comes from 'git format-patch', which completely leaves the commit out from its output if it's empty, no empty commits are ever created by 'git am --rebasing'. By teaching 'git am --rebasing' a --keep-empty option and letting the caller decide whether or not to keep empty commits, we can unify the two different mechanisms that git-rebase--am.sh uses for rebasing. --- git-am.sh | 10 +- git-rebase--am.sh | 20 ++-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/git-am.sh b/git-am.sh index b6a5300..37641b7 100755 --- a/git-am.sh +++ b/git-am.sh @@ -37,7 +37,8 @@ abort restore the original branch and abort the patching operation. committer-date-is-author-datelie about committer date ignore-date use current timestamp for author date rerere-autoupdate update the index with reused conflict resolution if possible -rebasing* (internal use for git-rebase) +rebasing* (internal use for git-rebase) +keep-empty* (internal use for git-rebase) . git-sh-setup . git-sh-i18n @@ -375,6 +376,7 @@ git_apply_opt= committer_date_is_author_date= ignore_date= allow_rerere_autoupdate= +keep_empty= if test $(git config --bool --get am.keepcr) = true then @@ -414,6 +416,8 @@ do abort=t ;; --rebasing) rebasing=t threeway=t ;; + --keep-empty) + keep_empty=t ;; -d|--dotest) die $(gettext -d option is no longer supported. Do not use.) ;; @@ -669,6 +673,10 @@ do echo $commit $dotest/original-commit get_author_ident_from_commit $commit $dotest/author-script git diff-tree --root --binary $commit $dotest/patch + test -s $dotest/patch || test -n $keep_empty || { + go_next + continue + } else git mailinfo $keep $no_inbody_headers $scissors $utf8 $dotest/msg $dotest/patch \ $dotest/$msgnum $dotest/info || diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 392ebc9..37c1b23 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -17,20 +17,12 @@ skip) esac test -n $rebase_root root_flag=--root - -if test -n $keep_empty -then - # we have to do this the hard way. git format-patch completely squashes - # empty commits and even if it didn't the format doesn't really lend - # itself well to recording empty patches. fortunately, cherry-pick - # makes this easy - git cherry-pick --allow-empty $revisions -else - git format-patch -k --stdout --full-index --ignore-if-in-upstream \ - --src-prefix=a/ --dst-prefix=b/ \ - --no-renames $root_flag $revisions | - git am $git_am_opt --rebasing --resolvemsg=$resolvemsg -fi move_to_original_branch +test -n $keep_empty git_am_opt=$git_am_opt --keep-empty +git format-patch -k --stdout --full-index --ignore-if-in-upstream \ + --src-prefix=a/ --dst-prefix=b/ \ + --no-renames $root_flag $revisions | +git am $git_am_opt --rebasing --resolvemsg=$resolvemsg +move_to_original_branch ret=$? test 0 != $ret -a -d $state_dir write_basic_state -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] git-rebase--interactive.sh: look up subject in add_pick_line
Thanks for reviewing. On Fri, Jul 20, 2012 at 1:14 AM, Johannes Sixt j.s...@viscovery.net wrote: Am 7/18/2012 9:27, schrieb Martin von Zweigbergk: @@ -814,7 +814,8 @@ add_pick_line () { else comment_out= fi - printf '%s\n' ${comment_out}pick $1 $2 $todo + line=$(git rev-list -1 --pretty=oneline --abbrev-commit --abbrev=7 $1) + printf '%s\n' ${comment_out}pick $line $todo I don't like this. On Windows, rebase -i is already slow, and these extra processes will make it even slower. I don't like it either :-(. Anything that can be done about this? Perhaps the rev-list call can generate all of the full SHA1, the short SHA1, and the subject with a --pretty format? After patch 7/7, cherry is used instead of rev-list. Ideally, I would have liked to teach git rev-list --cherry-pick to somehow use a limit just like cherry does, but I couldn't think of a generic way of doing that (in this case, we want to say something like range a..b, but drop commits that are equivalent to any in b..c). I actually don't remember if I gave up because I couldn't think of a sensible way of specifying ranges like that, or if I just ran out of time (not familiar with the revision-walking code). Now it seems to me that something like git rev-list a..b --not-cherry-picks b..c makes sense, but maybe it's just too specific and we should just support the limited (no pun intended) case we need to emulate git cherry, i.e. something like git rev-list --cherry-with-limit=a c...b. Feedback appreciated. Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] rebase (without -p): correctly calculate patches to rebase
On Fri, Jul 20, 2012 at 1:18 AM, Johannes Sixt j.s...@viscovery.net wrote: Am 7/18/2012 9:27, schrieb Martin von Zweigbergk: diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 37c1b23..fe3fdd1 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -16,11 +16,9 @@ skip) ;; esac -test -n $rebase_root root_flag=--root test -n $keep_empty git_am_opt=$git_am_opt --keep-empty -git format-patch -k --stdout --full-index --ignore-if-in-upstream \ - --src-prefix=a/ --dst-prefix=b/ \ - --no-renames $root_flag $revisions | +generate_revisions | +sed -e 's/\([0-9a-f]\{40\}\)/From \1 Mon Sep 17 00:00:00 2001/' | git am $git_am_opt --rebasing --resolvemsg=$resolvemsg move_to_original_branch Just curious (as all tests pass): What does this do? It looks like format-patch is not called anymore and git-am sees only SHA1s. Does it force git-am to cherry-pick the patches? That probably deserves to be mentioned in the commit message. Or maybe in as a comment in the code. Either way, since 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26), 'git am --rebasing' never looks at anything but the sha1, so most of the output from 'git format-patch' is currently ignored. It doesn't do cherry-pick, though, but runs 'git diff-tree' and other commands and then feeds the result to 'git apply', just like a regular 'git am' invocation would. Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/7] Remove unused and bad gettext block from git-am
On Tue, Jul 24, 2012 at 11:27 AM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Jiang Xin wrote: Gettext message should not start with '-' nor '--'. Since the '-d' and '--dotest' options do not exist in OPTIONS_SPEC variable, it's safe to remove the block. The above justification is not a sufficient reason to stop giving helpful advice when someone uses an option that was historically supported: I think Jiang is saying that git am --dotest=... already errors out because dotest is not in the OPTIONS_SPEC. See 98ef23b (git-am: minor cleanups, 2009-01-28). Or am I missing something? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jul 2012, #08; Thu, 26)
On Thu, Jul 26, 2012 at 11:09 PM, Junio C Hamano gits...@pobox.com wrote: * mz/rebase-range (2012-07-18) 7 commits - rebase (without -p): correctly calculate patches to rebase - rebase -p: don't request --left-right only to ignore left side - rebase -p: use --cherry-mark for todo file - git-rebase--interactive.sh: look up subject in add_pick_line - git-rebase--interactive: group all $preserve_merges code - git-rebase--interactive.sh: extract function for adding pick line - git-rebase--am.sh: avoid special-casing --keep-empty Expecting a reroll. Yep, will try to get some time for this soon. Will probably try using patch-id as you suggested. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] remove unnecessary parameter from get_patch_ids()
get_patch_ids() takes an already initialized rev_info and a prefix. The prefix is used when initalizing a second rev_info. Since the initialized rev_info already has a prefix and it seems the prefix doesn't change, we can used the prefix from the initialized rev_info to initialize the second rev_info. Signed-off-by: Martin von Zweigbergk martin.von.zweigbe...@gmail.com --- builtin/log.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index ecc2793..7a92e3f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -696,7 +696,7 @@ static int reopen_stdout(struct commit *commit, const char *subject, return 0; } -static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const char *prefix) +static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids) { struct rev_info check_rev; struct commit *commit; @@ -717,7 +717,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha init_patch_ids(ids); /* given a range a..b get all patch ids for b..a */ - init_revisions(check_rev, prefix); + init_revisions(check_rev, rev-prefix); o1-flags ^= UNINTERESTING; o2-flags ^= UNINTERESTING; add_pending_object(check_rev, o1, o1); @@ -1306,7 +1306,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (hashcmp(o[0].item-sha1, o[1].item-sha1) == 0) return 0; } - get_patch_ids(rev, ids, prefix); + get_patch_ids(rev, ids); } if (!use_stdout) @@ -1525,7 +1525,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) return 0; } - get_patch_ids(revs, ids, prefix); + get_patch_ids(revs, ids); if (limit add_pending_commit(limit, revs, UNINTERESTING)) die(_(Unknown commit %s), limit); -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] log: remove redundant check for merge commit
On Fri, Jul 27, 2012 at 3:30 PM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martin.von.zweigbe...@gmail.com writes: Also re-initializing rev_info fields to the same values already set in init_revisions(). Oops, that should have been _avoid_ re-initializing. I suspect that explicit initialization to revs.ignore_merges outself can be called belt-and-braces defensive programming to avoid getting surprised by future changes to what init_revisions() would do, so I do not have strong preference either way. I suspected the same, but OTOH, if it was ever changed, one would have to go through all call sites anyway. I checked the other calls to init_revisions (48 of them, I think) and I found only 3 other places where a field was re-initialized to the same value. In this case it was done inconsistently even within the file. Seeing init_revisions() followed by revs.ignore_merges = 1; in one place but not the other can easily lead one to believe that the other place does not ignore merges. Do you want a reroll with updated commit messages (the missing avoid above, the dropped seems like about the prefix in 1/2)? Since you don't have a strong preference about the explicit initialization, I assume I can leave those hunks in. I would clarify my reasoning, though. Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] log: remove redundant check for merge commit
Sorry, I meant to CC the list. See below. On Sat, Jul 28, 2012 at 9:56 PM, Martin von Zweigbergk martin.von.zweigbe...@gmail.com wrote: On Fri, Jul 27, 2012 at 11:52 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: It seems to have some interaction with your other topic, though. These two patches alone will pass the existing tests, but merging it with mz/rebase-range breaks t3412. I didn't look into it, but perhaps this breaks git cherry in some way? Yes, it breaks git cherry quite badly, by not ignoring merges at all. I incorrectly assumed that ignore_merges was about revision traversal, but now I think it's only diff output from 'git log' (and possibly others). What I think tricked me was seeing that ignore_merges = 1 closely followed by a comment saying ignore merges. But now I think the explicit code to ignore merges is necessary (as show by the failing test case), but can be replaced by rev_info.max_parents = 1. Setting ignore_merges = 1, OTOH, now seems doubly redundant: not only does it set the same value as was set in init_revisions, but it's also irrelevant. Since cherry doesn't generate any diff output, I think ignore_merges is never used. Flipping the values of all of ignore_merges, combine_merges and diff does not have any effect on test cases at least. I hope my explanation makes some sense at least... I'll send a reroll when I get time. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] cherry: don't set ignored rev_info options
Ever since cherry was built-in in e827633 (Built-in cherry, 2006-10-24), it has set a bunch of options on the the rev_info that are only used while outputting a patch. But since the built-in cherry command never needs to output any patch (it uses add_commit_patch_id and has_commit_patch_id instead), these options are just distractions, so remove them. Signed-off-by: Martin von Zweigbergk martin.von.zweigbe...@gmail.com --- builtin/log.c | 4 1 file changed, 4 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 7a92e3f..8cea1e5 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1508,10 +1508,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) } init_revisions(revs, prefix); - revs.diff = 1; - revs.combine_merges = 0; - revs.ignore_merges = 1; - DIFF_OPT_SET(revs.diffopt, RECURSIVE); if (add_pending_commit(head, revs, 0)) die(_(Unknown commit %s), head); -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] remove unnecessary parameter from get_patch_ids()
get_patch_ids() takes an already initialized rev_info and a prefix. The prefix is used when initalizing a second rev_info. Since the initialized rev_info already has a prefix and the prefix never changes, we can used the prefix from the initialized rev_info to initialize the second rev_info. Signed-off-by: Martin von Zweigbergk martin.von.zweigbe...@gmail.com --- builtin/log.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index ecc2793..7a92e3f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -696,7 +696,7 @@ static int reopen_stdout(struct commit *commit, const char *subject, return 0; } -static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const char *prefix) +static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids) { struct rev_info check_rev; struct commit *commit; @@ -717,7 +717,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha init_patch_ids(ids); /* given a range a..b get all patch ids for b..a */ - init_revisions(check_rev, prefix); + init_revisions(check_rev, rev-prefix); o1-flags ^= UNINTERESTING; o2-flags ^= UNINTERESTING; add_pending_object(check_rev, o1, o1); @@ -1306,7 +1306,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (hashcmp(o[0].item-sha1, o[1].item-sha1) == 0) return 0; } - get_patch_ids(rev, ids, prefix); + get_patch_ids(rev, ids); } if (!use_stdout) @@ -1525,7 +1525,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) return 0; } - get_patch_ids(revs, ids, prefix); + get_patch_ids(revs, ids); if (limit add_pending_commit(limit, revs, UNINTERESTING)) die(_(Unknown commit %s), limit); -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] Small log simplifications
Separated out the removal of the unused diff options into patch 2/3 and added the necessary max_parents=1 in patch 3/3. Martin von Zweigbergk (3): remove unnecessary parameter from get_patch_ids() cherry: don't set ignored rev_info options log: remove redundant check for merge commit builtin/log.c | 22 ++ 1 file changed, 6 insertions(+), 16 deletions(-) -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] log: remove redundant check for merge commit
While walking the revision list in get_patch_ids and cmd_cherry, we check for each commit if there is more than one parent and ignore the commit if that is the case. Instead, set rev_info.max_parents to 1 and let the revision traversal code handle it for us. Signed-off-by: Martin von Zweigbergk martin.von.zweigbe...@gmail.com --- builtin/log.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 8cea1e5..3423d11 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -718,6 +718,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids) /* given a range a..b get all patch ids for b..a */ init_revisions(check_rev, rev-prefix); + check_rev.max_parents = 1; o1-flags ^= UNINTERESTING; o2-flags ^= UNINTERESTING; add_pending_object(check_rev, o1, o1); @@ -726,10 +727,6 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids) die(_(revision walk setup failed)); while ((commit = get_revision(check_rev)) != NULL) { - /* ignore merges */ - if (commit-parents commit-parents-next) - continue; - add_commit_patch_id(commit, ids); } @@ -1508,6 +1505,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) } init_revisions(revs, prefix); + revs.max_parents = 1; if (add_pending_commit(head, revs, 0)) die(_(Unknown commit %s), head); @@ -1530,10 +1528,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) if (prepare_revision_walk(revs)) die(_(revision walk setup failed)); while ((commit = get_revision(revs)) != NULL) { - /* ignore merges */ - if (commit-parents commit-parents-next) - continue; - commit_list_insert(commit, list); } -- 1.7.11.1.104.ge7b44f1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] am: suppress error output from a conditional
On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra artag...@gmail.com wrote: When testing if the $dotest directory exists, and if $next is greater than $last When can that happen? If one edits the todo? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] rebase -i: don't error out if $state_dir already exists
On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Practically speaking, the only reason why a `mkdir $state_dir` would fail is because $state_dir already exists. Would we ever get to this point in the code if it already exists? Also, I had the feeling that the check it might fail if the user does not have permissions to create the directory. Is there always something else that will fail first? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping
On Tue, Apr 23, 2013 at 7:02 AM, Ramkumar Ramachandra artag...@gmail.com wrote: diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index cc3a9a7..9514e31 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -597,7 +597,7 @@ do_next () { fi ;; esac - test -s $todo return + test -s $todo return 1 Unlike the other cases below, this seems to be replacing success by failure. What does it mean in practice that $todo is empty? comment_for_reflog finish newhead=$(git rev-parse HEAD) @@ -623,17 +623,15 @@ do_next () { $GIT_DIR/hooks/post-rewrite rebase $rewritten_list true # we don't care if this hook failed fi - rm -rf $state_dir - git gc --auto warn Successfully rebased and updated $head_name. - exit + return 0 So after this patch, the warning will coming before gc is run. It's a change, but it seems fine. gc usually only prints a few line, right? } do_rest () { while : do - do_next + do_next break done } Normally one would break if unsuccessful. What would fail if this was replaced by do_next || break and the above .. return 1 was .. return. I assume that was your first attempt, but why did it not work? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: use reflog to find common base with upstream
On Wed, Oct 16, 2013 at 11:53 AM, John Keeping j...@keeping.me.uk wrote: Commit 15a147e (rebase: use @{upstream} if no upstream specified, 2011-02-09) says: Make it default to 'git rebase @{upstream}'. That is also what 'git pull [--rebase]' defaults to, so it only makes sense that 'git rebase' defaults to the same thing. but that isn't actually the case. Since commit d44e712 (pull: support rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually chosen the most recent reflog entry which is an ancestor of the current branch if it can find one. It is exactly this inconsistency between git rebase and git pull --rebase that confused me enough to make me send my first email to this list almost 4 years ago [1], so thanks for working on this! I finished that thread with: Would it make sense to teach git rebase the same tricks as git pull --rebase? Then it took me a year before I sent a patch not unlike this one [2]. To summarize, the patch did not get accepted then because it makes rebase a little slower (or a lot slower in some cases). git pull --rebase is of course at least as slow in the same cases, but because it often involves connecting to a remote host, people would probably blame the connection rather than git itself even in those rare (?) cases. I think git merge-base HEAD $(git rev-list -g $upstream_name) is roughly correct and hopefully fast enough. That can lead to too long a command line, so I was planning on teaching merge-base a --stdin option, but never got around to it. Martin [1] http://thread.gmane.org/gmane.comp.version-control.git/136339 [2] http://thread.gmane.org/gmane.comp.version-control.git/166710 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: use reflog to find common base with upstream
On Wed, Oct 16, 2013 at 11:53 AM, John Keeping j...@keeping.me.uk wrote: Commit 15a147e (rebase: use @{upstream} if no upstream specified, 2011-02-09) says: Make it default to 'git rebase @{upstream}'. That is also what 'git pull [--rebase]' defaults to, so it only makes sense that 'git rebase' defaults to the same thing. but that isn't actually the case. Since commit d44e712 (pull: support rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually chosen the most recent reflog entry which is an ancestor of the current branch if it can find one. Change rebase so that it uses the same logic. Signed-off-by: John Keeping j...@keeping.me.uk --- git-rebase.sh | 8 t/t3400-rebase.sh | 6 -- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 226752f..fd36cf7 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -437,6 +437,14 @@ then error_on_missing_default_upstream rebase rebase \ against git rebase branch fi + for reflog in $(git rev-list -g $upstream_name 2/dev/null) + do + if test $reflog = $(git merge-base $reflog HEAD) + then + upstream_name=$reflog + break + fi + done ;; *) upstream_name=$1 shift A little later, onto_name gets assigned like so: onto_name=${onto-$upstream_name} So if upstream_name was set above, then onto would get the same value, which is not what we want, right? It seems like this block of code should come a bit later. I also think it not be run only when rebase was run without a given upstream. If the configured upstream is origin/master, it seems like it would be surprising to get different behavior from git rebase and git rebase origin/master. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: use reflog to find common base with upstream
On Mon, Oct 21, 2013 at 4:24 AM, John Keeping j...@keeping.me.uk wrote: On Sun, Oct 20, 2013 at 10:03:29PM -0700, Martin von Zweigbergk wrote: On Wed, Oct 16, 2013 at 11:53 AM, John Keeping j...@keeping.me.uk wrote: Commit 15a147e (rebase: use @{upstream} if no upstream specified, 2011-02-09) says: Make it default to 'git rebase @{upstream}'. That is also what 'git pull [--rebase]' defaults to, so it only makes sense that 'git rebase' defaults to the same thing. but that isn't actually the case. Since commit d44e712 (pull: support rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually chosen the most recent reflog entry which is an ancestor of the current branch if it can find one. It is exactly this inconsistency between git rebase and git pull --rebase that confused me enough to make me send my first email to this list almost 4 years ago [1], so thanks for working on this! I finished that thread with: Would it make sense to teach git rebase the same tricks as git pull --rebase? Then it took me a year before I sent a patch not unlike this one [2]. To summarize, the patch did not get accepted then because it makes rebase a little slower (or a lot slower in some cases). git pull --rebase is of course at least as slow in the same cases, but because it often involves connecting to a remote host, people would probably blame the connection rather than git itself even in those rare (?) cases. I think git merge-base HEAD $(git rev-list -g $upstream_name) is roughly correct and hopefully fast enough. That can lead to too long a command line, so I was planning on teaching merge-base a --stdin option, but never got around to it. I'm not sure we should worry about the additional overhead here. In the common case, we should hit a common ancestor within the first couple of reflog entries; and in the case that will be slow, it's likely that there are a lot of differences between the branches so the cherry comparison phase will take a while anyway. Perhaps true. I created a simple commit based on my origin/master@{1} in git.git, which happened to be 136 commits behind origin/master. Before (a modified version of) your patch, it took 0.756s to rebase it (best of 5) and afterwards it took 0.720s. And in a worse case: The same test with one commit off my origin/master@{13}, 2910 behind origin/master, shows an increase from 2.75s to 4.04s. And a degenerate case: I created a test branch (called u) with 1000-entry reflog from the output of git rev-list --first-parent origin/master | head -1000 | tac and created the same simple commit as before off of the end of this reflog (u@{999}). This ended up 3769 commits behind u@{0} (aka origin/master). In this case it went from 3.43s to 3m32s. Obviously, this was a degenerate case designed to be slow, but I think it's still worth noting that one can get such O(n^2) behavior e.g. if one lets a branch get out of sync with an upstream that's very frequently fetches (I've heard of people running short-interval cron jobs that fetch from a remote). I do like the feature, but I'm still concerned about this last case. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* Bug report: reset -p HEAD
Sorry about the regression and thanks for report and fixes. On Thu, Oct 24, 2013 at 9:24 PM, Jeff King p...@peff.net wrote: On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote: Maarten de Vries maar...@de-vri.es writes: Some more info: It used to work as intended. Using a bisect shows it has been broken by commit 166ec2e9. Thanks. A knee-jerk change without thinking what side-effect it has for you to try out. builtin/reset.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index f2f9d55..a3088d9 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (patch_mode) { if (reset_type != NONE) die(_(--patch is incompatible with --{hard,mixed,soft})); - return run_add_interactive(sha1_to_hex(sha1), --patch=reset, pathspec); + return run_add_interactive( + (unborn || strcmp(rev, HEAD)) + ? sha1_to_hex(sha1) + : HEAD, --patch=reset, pathspec); } I think that's the correct fix for the regression. You are restoring the original, pre-166ec2e9 behavior for just the HEAD case. I do not think add--interactive does any other magic between a symbolic rev and its sha1, except for recognizing HEAD specially. However, if you wanted to minimize the potential impact of 166ec2e9, you could pass the sha1 _only_ in the unborn case, like this: Plus, the end result is more readable, IMHO. diff --git a/builtin/reset.c b/builtin/reset.c index f2f9d55..bfdd8d3 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -283,6 +283,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (unborn) { /* reset on unborn branch: treat as reset to empty tree */ hashcpy(sha1, EMPTY_TREE_SHA1_BIN); + rev = EMPTY_TREE_SHA1_HEX; } else if (!pathspec.nr) { struct commit *commit; if (get_sha1_committish(rev, sha1)) @@ -304,7 +305,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (patch_mode) { if (reset_type != NONE) die(_(--patch is incompatible with --{hard,mixed,soft})); - return run_add_interactive(sha1_to_hex(sha1), --patch=reset, pathspec); + return run_add_interactive(rev, --patch=reset, pathspec); } /* git reset tree [--] paths... can be used to That fixes any possible regression from add--interactive treating the two cases differently. On an unborn branch, we will still say apply this hunk rather than unstage this hunk. That's not a regression, because it simply didn't work before, but it's not ideal. To fix that, we need to somehow tell add--interactive this is HEAD, but use the empty tree because it's unborn. I can think of a few simple-ish ways: 1. Pass the head/not-head flag as a separate option. 2. Pass HEAD even in the unborn case; teach add--interactive to convert an unborn HEAD to the empty tree. 3. Teach add--interactive to recognize the empty tree sha1 as an unstage path. I kind of like (3). At first glance, it is wrong; we will also treat git reset -p $(git hash-object -t tree /dev/null) as if HEAD had been passed. But if you are explicitly passing the empty tree like that, I think saying unstage makes a lot of sense. Makes sense to me. I'm sure others can implement that much faster than I can, but I feel a little guilty, so I'm happy to do it if no one else wants to, as long as we agree this is the way we want to go. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach --fork-point mode
Thanks for taking care of this! Maybe John or I can finally get the changes to rebase done after this. A few comments below. Sorry I didn't find time to review the earlier revisions. On Fri, Oct 25, 2013 at 2:38 PM, Junio C Hamano gits...@pobox.com wrote: + +where `origin/master` used to point at commits B3, B2, B1 and now it +points at B, and your `topic` branch was stated on top of it back s/stated/started/ + bases = get_merge_bases_many(derived, revs.nr, revs.commit, 0); + + /* +* There should be one and only one merge base, when we found +* a common ancestor among reflog entries. +*/ + if (!bases || bases-next) + return 1; + + /* And the found one must be one of the reflog entries */ + for (i = 0; i revs.nr; i++) + if (bases-item-object == revs.commit[i]-object) + break; /* found */ + if (revs.nr = i) + return 1; /* not found */ + + printf(%s\n, sha1_to_hex(bases-item-object.sha1)); + free_commit_list(bases); + return 0; Should free_commit_list also be called in the two return 1 cases above? I suppose the process will exit soon after this, but that seems to be true for all three cases. diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh index f80bba8..4f09db0 100755 --- a/t/t6010-merge-base.sh +++ b/t/t6010-merge-base.sh @@ -230,4 +230,31 @@ test_expect_success 'criss-cross merge-base for octopus-step' ' test_cmp expected.sorted actual.sorted ' +test_expect_success 'using reflog to find the fork point' ' + git reset --hard + git checkout -b base $E + + ( + for count in 1 2 3 4 5 + do + git commit --allow-empty -m Base commit #$count + git rev-parse HEAD expect$count + git checkout -B derived + git commit --allow-empty -m Derived #$count + git rev-parse HEAD derived$count + git checkout base || exit 1 I think this creates a history like ---E---B1--B2--B3--B4--B5 (base) \ \ \ \ \ D1 D2 D3 D4 D5 (derived) So I think the following test would pass even if you drop the --fork-point. Did you mean to create a fan-shaped history by resetting base to $E on every iteration above? + git merge-base --fork-point base $(cat derived$count) actual + test_cmp expect$count actual || exit 1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/16] add: avoid yoda conditions
I was recently confused by the yoda condition in this block of code from [1] + for (i = 0; i revs.nr; i++) + if (bases-item-object == revs.commit[i]-object) + break; /* found */ + if (revs.nr = i) I think I was particularly surprised because it came so soon after the i revs.nr. I didn't bother commenting because it seemed too subjective and the code base has tons of these. Something as simple as git grep '[0-9] []' *.c finds a bunch (probably with lots of false positives and negatives). I guess what I'm trying to say is that either we accept them and get used to reading them without being surprised, or we can change a bit more than one at a time perhaps? I understand that this was an occurrence you just happened to run into, and I'm not saying that a patch has to deal with _all_ occurrences. I'm more just wondering if we want mention our position, whatever it is, in CodingGuidelines. Martin [1] http://thread.gmane.org/gmane.comp.version-control.git/236252/focus=236716 On Thu, Oct 31, 2013 at 2:25 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 226f758..9b30356 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -429,7 +429,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) argc--; argv++; - if (0 = addremove_explicit) + if (addremove_explicit = 0) addremove = addremove_explicit; else if (take_worktree_changes ADDREMOVE_DEFAULT) addremove = 0; /* -u was given but not -A */ -- 1.8.4.2+fc1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] rebase: use reflog to find common base with upstream
On Sun, Dec 8, 2013 at 12:06 PM, John Keeping j...@keeping.me.uk wrote: Commit 15a147e (rebase: use @{upstream} if no upstream specified, 2011-02-09) says: Make it default to 'git rebase @{upstream}'. That is also what 'git pull [--rebase]' defaults to, so it only makes sense that 'git rebase' defaults to the same thing. but that isn't actually the case. Since commit d44e712 (pull: support rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually chosen the most recent reflog entry which is an ancestor of the current branch if it can find one. In my mind, 'git pull --rebase' does default to @{u}, it just started interpreting it differently in d44e712, but maybe I'm just being defensive :-). In a similar way, I think your patch is about interpreting the upstream argument differently, not about changing the default upstream argument. This is why I think git rebase and git rebase origin/master should be the same (when origin/master is the configured upstream). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html