* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Jeff King @ 2009-12-08 5:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James Vega, git
In-Reply-To: <7v3a3mqhhd.fsf@alter.siamese.dyndns.org>
On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:
> An update. I tried your reproduction recipe with 1.6.5.2 and it doesn't
> reproduce, but with 1.6.5.3 it does.
Thanks, both, for a very helpful bug report. 24ab81a was totally bogus,
but we lacked a test for deleting a non-empty file. That test and a fix
for the problem are in the patch below.
I am still slightly concerned that James's
git diff | sed '/^deleted file/d' | git apply --cached
behaves as it does. What should git-apply do with a patch like:
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
@@ -1 +0,0 @@
-foo
? I can see either turning it into a deletion patch (because /dev/null
is special) or barfing (because /dev/null as a special case should have
appeared in the "diff" line). But creating a dev/null file seems very
wrong.
But maybe it is not worth worrying about too much. That patch format is
not generated intentionally by any known software.
Here is the fix directly on top of 24ab81a.
-- >8 --
Subject: [PATCH] add-interactive: fix deletion of non-empty files
Commit 24ab81a fixed the deletion of empty files, but broke
deletion of non-empty files. The approach it took was to
factor out the "deleted" line from the patch header into its
own hunk, the same way we do for mode changes. However,
unlike mode changes, we only showed the special "delete this
file" hunk if there were no other hunks. Otherwise, the user
would annoyingly be presented with _two_ hunks: one for
deleting the file and one for deleting the content.
Instead, this patch takes a separate approach. We leave the
deletion line in the header, so it will be used as usual by
non-empty files if their deletion hunk is staged. For empty
files, we create a deletion hunk with no content; it doesn't
add anything to the patch, but by staging it we trigger the
application of the header, which does contain the deletion.
Signed-off-by: Jeff King <peff@peff.net>
---
There is a slightly different approach we could take, too: keep the
"deletion" hunk as a first-class hunk, and just meld the content hunk's
output into it. Then both cases would get the "Stage deletion" question
instead of the "Stage this hunk" you get now for non-empty files (which
just happens to trigger a deletion due to the headers).
That would take some refactoring, though, as pulling the deletion hunk
out means we are re-ordering the headers. So right now if you did that
your ($head, @hunk) output would be something like:
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
deleted file mode 100644
@@ -1 +0,0 @@
-foo
which is pretty weird. On the other hand, we already do that funny
ordering for mode hunks, and git-apply is just fine with it. A mode hunk
with content change looks like this:
diff --git a/foo b/foo
index 257cc56..19c6cc1
--- a/foo
+++ b/foo
old mode 100644
new mode 100755
And it also opens the door to editing the hunk to stop the deletion, but
still tweak the content change. Right now if you edit a deletion patch,
you can't remove the 'deleted' bit, and if your edit result keeps any
content in the file, apply will complain. I'm not sure that particular
feature would be useful though (I have certainly never wanted it).
git-add--interactive.perl | 18 +++++++++++-------
t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..f4b95b1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -731,17 +731,19 @@ sub parse_diff_header {
my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
- my $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };
+ my $is_deletion;
for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
my $dest =
$src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ? $mode :
- $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :
$head;
push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
+ if ($src->{TEXT}->[$i] =~ /^deleted file/) {
+ $is_deletion = 1;
+ }
}
- return ($head, $mode, $deletion);
+ return ($head, $mode, $is_deletion);
}
sub hunk_splittable {
@@ -1209,7 +1211,7 @@ sub patch_update_file {
my ($ix, $num);
my $path = shift;
my ($head, @hunk) = parse_diff($path);
- ($head, my $mode, my $deletion) = parse_diff_header($head);
+ ($head, my $mode, my $is_deletion) = parse_diff_header($head);
for (@{$head->{DISPLAY}}) {
print;
}
@@ -1217,8 +1219,8 @@ sub patch_update_file {
if (@{$mode->{TEXT}}) {
unshift @hunk, $mode;
}
- if (@{$deletion->{TEXT}} && !@hunk) {
- @hunk = ($deletion);
+ if ($is_deletion && !@hunk) {
+ @hunk = ({TEXT => [], DISPLAY => [], TYPE => 'deletion'});
}
$num = scalar @hunk;
@@ -1441,14 +1443,16 @@ sub patch_update_file {
@hunk = coalesce_overlapping_hunks(@hunk);
my $n_lofs = 0;
+ my $hunks_used = 0;
my @result = ();
for (@hunk) {
if ($_->{USE}) {
push @result, @{$_->{TEXT}};
+ $hunks_used++;
}
}
- if (@result) {
+ if ($hunks_used) {
my $fh;
my @patch = (@{$head->{TEXT}}, @result);
my $apply_routine = $patch_mode_flavour{APPLY};
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index aa5909b..0926b91 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '
'
cat >expected <<EOF
+diff --git a/non-empty b/non-empty
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/non-empty
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+test_expect_success 'deleting a non-empty file' '
+ git reset --hard &&
+ echo content >non-empty &&
+ git add non-empty &&
+ git commit -m non-empty &&
+ rm non-empty &&
+ echo y | git add -p non-empty &&
+ git diff --cached >diff &&
+ test_cmp expected diff
+'
+
+cat >expected <<EOF
diff --git a/empty b/empty
deleted file mode 100644
index e69de29..0000000
--
1.6.5.1.g24ab.dirty
^ permalink raw reply related
* Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions
From: Jeff King @ 2009-12-08 5:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <7vein635vn.fsf@alter.siamese.dyndns.org>
On Mon, Dec 07, 2009 at 12:07:24PM -0800, Junio C Hamano wrote:
> I haven't asked people why they choose to write like this:
>
> char* string;
>
> beyond "that is how we were taught and what we are used to".
I have seen it in C++ code and recommended many years ago on
comp.lang.c++. The argument was something along the lines of:
1. It's good to keep type information together, especially in C++
where you are often doing things like using types as template
parameters.
2. The fact that "char* foo, bar" doesn't do what you want isn't
relevant if you have a style guideline not to declare two variables
on the same line (because it's easier to notice both if they each
get their own line, and because in C++ you can declare closer to
the point of use).
But that is me paraphrasing an argument I read on usenet almost 10 years
ago, so I may be entirely misremembering (and please don't flame me; I
am presenting it for anthropological curiosity, not because I believe we
should use that style).
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2009, #02; Sat, 05)
From: Tay Ray Chuan @ 2009-12-08 5:58 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Martin Storsj?, Junio C Hamano, git
In-Reply-To: <20091207153736.GC17173@spearce.org>
Hi,
On Mon, Dec 7, 2009 at 11:37 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Martin Storsj? <martin@martin.st> wrote:
>> On Sun, 6 Dec 2009, Junio C Hamano wrote:
>> >
>> > * tr/http-updates (2009-12-01) 3 commits
>> > - Allow curl to rewind the RPC read buffer
>> > - Add an option for using any HTTP authentication scheme, not only basic
>> > - http: maintain curl sessions
>> >
>> > There was a discussion on a better structure not to require rewinding in
>> > the first place? I didn't follow it closely...
>>
>> I think the conclusion is: Rewinding support isn't strictly necessary,
>> there's a number of mechanisms in both git and curl that should make sure
>> that those cases shouldn't surface. A few of them in curl have an
>> unfortunate conincidence of bugs up until the latest version, though,
>> leaving much fewer mechanisms in place to avoid this.
>>
>> Since that patch is quite non-intrusive I think it's a good safeguard,
>> though. What do you think, Tay, keep it or leave it?
>
> I think the conclusion of the thread was that what you have queued
> in tr/http-updates is OK as-is. The patch to grow the postbuffer
> to store the entire request wasn't a good idea and got dropped.
Martin, sorry the late reply. I agree with Shawn. Perhaps we could
relook at rewinding-to-any-position when restarting the rpc client
(eg. git-send-pack) has been implemented.
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Jeff King @ 2009-12-08 6:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James Vega, git
In-Reply-To: <20091208054724.GA21347@coredump.intra.peff.net>
On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:
> There is a slightly different approach we could take, too: keep the
> "deletion" hunk as a first-class hunk, and just meld the content hunk's
> output into it. Then both cases would get the "Stage deletion" question
> instead of the "Stage this hunk" you get now for non-empty files (which
> just happens to trigger a deletion due to the headers).
BTW, the code for this is the much smaller change below. If you prefer
that, I can squash in the test and write up an appropriate commit
message.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..02e97b9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1217,7 +1217,11 @@ sub patch_update_file {
if (@{$mode->{TEXT}}) {
unshift @hunk, $mode;
}
- if (@{$deletion->{TEXT}} && !@hunk) {
+ if (@{$deletion->{TEXT}}) {
+ foreach my $hunk (@hunk) {
+ push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
+ push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
+ }
@hunk = ($deletion);
}
^ permalink raw reply related
* Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"
From: Nanako Shiraishi @ 2009-12-08 6:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Matthieu Moy, Michael J Gruber,
Michael Haggerty, git
In-Reply-To: <7vtyw2p2ju.fsf@alter.siamese.dyndns.org>
Quoting Junio C Hamano <gitster@pobox.com>
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Teach a new option, --autosquash, to the interactive rebase.
>> When the commit log message begins with "!fixup ...", and there
>> is a commit whose title begins with the same ..., automatically
>> modify the todo list of rebase -i so that the commit marked for
>> squashing come right after the commit to be modified, and change
>> the action of the moved commit from pick to squash.
>>
>> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
>
> Hmph, did you forget to retitle the message, or keep in-body "Subject:"?
Sorry. Yes I did. Please amend it to -
Subject: rebase -i --autosquash: auto-squash commits
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply
* Re: [PATCH] Add commit.status, --status, and --no-status
From: Jeff King @ 2009-12-08 6:04 UTC (permalink / raw)
To: James P. Howard, II; +Cc: git
In-Reply-To: <1260225927-33612-1-git-send-email-jh@jameshoward.us>
On Mon, Dec 07, 2009 at 05:45:27PM -0500, James P. Howard, II wrote:
> This commit provides support for commit.status, --status, and
> --no-status, which control whether or not the git status information
> is included in the commit message template when using an editor to
> prepare the commit message. It does not affect the effects of a
> user's commit.template settings.
Thanks, this looks very cleanly done. The only complaint I would make is
that it should probably include a simple test case.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] git-rm doc: Describe how to sync index & work tree
From: Björn Gustavsson @ 2009-12-08 6:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpr6qze4i.fsf@alter.siamese.dyndns.org>
2009/12/7 Junio C Hamano <gitster@pobox.com>:
> Looks sensible.
>
> I think mentioning "add -u" in the same section as "commit -a" would be
> helpful, as these two are more for user's own development (as opposed to
> vendor-code-drop). I'd perhaps squash something like this in. Please say
> "yes", "don't, it is horrible", or something in between ;-)
Yes. :-)
--
Björn Gustavsson, Erlang/OTP, Ericsson AB
^ permalink raw reply
* Re: [PATCH 2/2] git-rm doc: Describe how to sync index & work tree
From: Björn Gustavsson @ 2009-12-08 6:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpr6qze4i.fsf@alter.siamese.dyndns.org>
2009/12/7 Junio C Hamano <gitster@pobox.com>:
> I think mentioning "add -u" in the same section as "commit -a" would be
> helpful, as these two are more for user's own development (as opposed to
> vendor-code-drop). I'd perhaps squash something like this in. Please say
> "yes", "don't, it is horrible", or something in between ;-)
Yes, but...
> -automatically notice and record all removals.
> +automatically notice and record all removals. `git add -u`
> +can be used for a similar effect without commiting.
s/commiting/committing/
--
Björn Gustavsson, Erlang/OTP, Ericsson AB
^ permalink raw reply
* Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions
From: Jeff King @ 2009-12-08 6:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <20091208055735.GA9951@coredump.intra.peff.net>
On Tue, Dec 08, 2009 at 12:57:35AM -0500, Jeff King wrote:
> On Mon, Dec 07, 2009 at 12:07:24PM -0800, Junio C Hamano wrote:
>
> > I haven't asked people why they choose to write like this:
> >
> > char* string;
> >
> > beyond "that is how we were taught and what we are used to".
>
> I have seen it in C++ code and recommended many years ago on
> comp.lang.c++. The argument was something along the lines of:
Perhaps I should have simply used google:
http://www2.research.att.com/~bs/bs_faq2.html#whitespace
-Peff
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: James Vega @ 2009-12-08 6:49 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20091208060109.GB9951@coredump.intra.peff.net>
On Tue, Dec 08, 2009 at 01:01:09AM -0500, Jeff King wrote:
> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:
>
> > There is a slightly different approach we could take, too: keep the
> > "deletion" hunk as a first-class hunk, and just meld the content hunk's
> > output into it. Then both cases would get the "Stage deletion" question
> > instead of the "Stage this hunk" you get now for non-empty files (which
> > just happens to trigger a deletion due to the headers).
>
> BTW, the code for this is the much smaller change below. If you prefer
> that, I can squash in the test and write up an appropriate commit
> message.
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 35f4ef1..02e97b9 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1217,7 +1217,11 @@ sub patch_update_file {
> if (@{$mode->{TEXT}}) {
> unshift @hunk, $mode;
> }
> - if (@{$deletion->{TEXT}} && !@hunk) {
> + if (@{$deletion->{TEXT}}) {
> + foreach my $hunk (@hunk) {
> + push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
> + push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
> + }
> @hunk = ($deletion);
> }
>
Thanks for the quick patches. This was similar to what I was working on, but
cleaner than what I had. Works well for me.
--
James
GPG Key: 1024D/61326D40 2003-09-02 James Vega <vega.james@gmail.com>
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Junio C Hamano @ 2009-12-08 7:11 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, James Vega, git
In-Reply-To: <20091208054724.GA21347@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:
>
>> An update. I tried your reproduction recipe with 1.6.5.2 and it doesn't
>> reproduce, but with 1.6.5.3 it does.
>
> Thanks, both, for a very helpful bug report. 24ab81a was totally bogus,
> but we lacked a test for deleting a non-empty file. That test and a fix
> for the problem are in the patch below.
Thanks.
> I am still slightly concerned that James's
>
> git diff | sed '/^deleted file/d' | git apply --cached
>
> behaves as it does. What should git-apply do with a patch like:
>
> diff --git a/foo b/foo
> index 257cc56..0000000
> --- a/foo
> +++ /dev/null
> @@ -1 +0,0 @@
> -foo
>
> ? I can see either turning it into a deletion patch (because /dev/null
> is special) or barfing (because /dev/null as a special case should have
> appeared in the "diff" line). But creating a dev/null file seems very
> wrong.
I was wondering about the same thing while bisecting. By the current
definition of "diff --git", removing the "deleted file" or "new file" line
makes the patch an invalid "git format diff". See the beginning of
parse_git_header() where we say "we don't guess" and initialize both
is_new and is_delete to false (and we flip them upon seeing "deleted file"
and "new file", but never with "/dev/null").
> But maybe it is not worth worrying about too much. That patch format is
> not generated intentionally by any known software.
I think some recent other SCMs produce what they claim to be "diff --git",
but I don't know if they implement the format correctly enough. I am not
worried about their implemention of binary patches (if they do not
implement it correctly they will most likely get garbage), but do they get
the abbreviated hash on the "index" line correctly? You can put garbage
on the line and most of the time it would work but it will break "am -3"
by breaking "apply --build-fake-ancestor".
I just checked "hg diff --git"; at least it shows "deleted file".
> That would take some refactoring, though, as pulling the deletion hunk
> out means we are re-ordering the headers. So right now if you did that
> your ($head, @hunk) output would be something like:
>
> diff --git a/foo b/foo
> index 257cc56..0000000
> --- a/foo
> +++ /dev/null
> deleted file mode 100644
> @@ -1 +0,0 @@
> -foo
>
> which is pretty weird.
I agree it is weird.
> And it also opens the door to editing the hunk to stop the deletion, but
> still tweak the content change. Right now if you edit a deletion patch,
> you can't remove the 'deleted' bit, and if your edit result keeps any
> content in the file, apply will complain. I'm not sure that particular
> feature would be useful though (I have certainly never wanted it).
Interesting. Does "add -p" (especially its [e]dit codepath) know enough
about what it is doing? If so, it should be able to add "deleted file" on
its own (and remove it when the result of editing and picking hunks makes
the patch a non-deletion). For example, if you have a two-liner in the
index and have deleted one line in the work tree, and run "add -p":
diff --git a/foo b/foo
index 3bd1f0e..257cc56 100644
--- a/foo
+++ b/foo
@@ -1,2 +1 @@
foo
-bar
you *should* be able to edit it into a patch that removes all lines.
Perhaps the "add -i" at the end should offer, after noticing that the
chosen and edited hunks will make the postimage an empty file, a chance
for the user to say "I not only want to remove the contents from the path,
but want to remove the path itself" in such a case?
I dunno.
^ permalink raw reply
* Re: Gui criticism
From: Marco Costalba @ 2009-12-08 7:12 UTC (permalink / raw)
To: Ram Rachum; +Cc: git
In-Reply-To: <loom.20091205T194800-496@post.gmane.org>
On Sat, Dec 5, 2009 at 19:51, Ram Rachum <cool-rr@cool-rr.com> wrote:
> Hello!
>
> This is my first time on this list. I'm a Python developer who's been using git
> for about a year. I generally like it, but I have several gripes about the GUI.
> (My development machine is on Windows XP.) Would specifying these criticisms be
> helpful to you?
>
Wrong approach. First do then ask. Is not so polite but is how it
works in real word. :-)
^ permalink raw reply
* Re: [PATCH] Add commit.status, --status, and --no-status
From: Junio C Hamano @ 2009-12-08 7:13 UTC (permalink / raw)
To: Jeff King; +Cc: James P. Howard, II, git
In-Reply-To: <20091208060415.GC9951@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Dec 07, 2009 at 05:45:27PM -0500, James P. Howard, II wrote:
>
>> This commit provides support for commit.status, --status, and
>> --no-status, which control whether or not the git status information
>> is included in the commit message template when using an editor to
>> prepare the commit message. It does not affect the effects of a
>> user's commit.template settings.
>
> Thanks, this looks very cleanly done. The only complaint I would make is
> that it should probably include a simple test case.
Yes. Also I am a _bit_ worried about the name "status", as the longer
term direction is to make "status" not "a preview of commit", may confuse
people who do read Release Notes.
^ permalink raw reply
* Re: How do you best store structured data in git repositories?
From: David Aguilar @ 2009-12-08 7:14 UTC (permalink / raw)
To: Sebastian Setzer; +Cc: git
In-Reply-To: <1260220821.3545.12.camel@nord26-amd64>
On Mon, Dec 07, 2009 at 10:20:21PM +0100, Sebastian Setzer wrote:
> On Thursday, Dec 03 2009 at 16:14 -0800, David Aguilar wrote:
> > On Wed, Dec 02, 2009 at 04:17:10PM -0500, Avery Pennarun wrote:
> > > On Wed, Dec 2, 2009 at 4:08 PM, Sebastian Setzer
> > > <sebastianspublicaddress@googlemail.com> wrote:
> > > > Do you use XML for this purpose?
> > >
> > > XML is terrible for most data storage purposes.
> >
> > I agree 100%.
> >
> > JSON's not too bad for data structures and is known to
> > be friendly to XML expats.
> >
> Sorry, I didn't want to start a flamewar against XML. I'm no big friend
> of XML myself, but I don't know of an (open source) diff-/merge tool for
> any general purpose file format other than XML or plain text.
> When you mention other formats, I'd be interested in
> - why this format is good for storage in git
> - if there are merge tools available which ensure that, after a merge,
> the structure (and maybe additional contraints) is still valid.
>
> Thanks for your comments,
> Sebastian
Sorry, didn't mean to sound xml-flaming. The only reason for
mentioning json, yaml, etc. is that they're good data structure
formats. They're all plain text formats, so you can use existing
diff/merge tools.
I guess none of this has much to do with git aside from being
able to write custom merge drivers to operate on them as data.
If there's a diff/merge tool for xml that works well then
hooking it up to git-{diff,merge}tool might be something
to try too.
--
David
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Junio C Hamano @ 2009-12-08 7:28 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, James Vega, git
In-Reply-To: <20091208060109.GB9951@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:
>
>> There is a slightly different approach we could take, too: keep the
>> "deletion" hunk as a first-class hunk, and just meld the content hunk's
>> output into it. Then both cases would get the "Stage deletion" question
>> instead of the "Stage this hunk" you get now for non-empty files (which
>> just happens to trigger a deletion due to the headers).
>
> BTW, the code for this is the much smaller change below. If you prefer
> that, I can squash in the test and write up an appropriate commit
> message.
Doubly interesting, as I recall reading "That would take some refactoring,
though, as pulling the deletion hunk"
... goes and looks ...
Ah, Ok, the "refactoring" refers to the "header reordering weirdness".
That might be something we may want to fix someday, when we find ourselves
needing to add a feature to turn deletion into non-deletion or vice versa
during "add -p" [e]dit, as I suspect that the "hunk editing" codepath does
not keep track of what the user's patch is doing, to the point that it
does not even know how many lines there are supposed to be in the
resulting hunk that it asks "git apply" to recount. There is no way to
add/delete "deleted file" line if the logic does not know what the patch
is doing.
But someday is not today. I think this six-liner is preferable.
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 35f4ef1..02e97b9 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1217,7 +1217,11 @@ sub patch_update_file {
> if (@{$mode->{TEXT}}) {
> unshift @hunk, $mode;
> }
> - if (@{$deletion->{TEXT}} && !@hunk) {
> + if (@{$deletion->{TEXT}}) {
> + foreach my $hunk (@hunk) {
> + push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
> + push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
> + }
> @hunk = ($deletion);
> }
>
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Jeff King @ 2009-12-08 7:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James Vega, git
In-Reply-To: <7vvdgindo3.fsf@alter.siamese.dyndns.org>
On Mon, Dec 07, 2009 at 11:11:08PM -0800, Junio C Hamano wrote:
> I was wondering about the same thing while bisecting. By the current
> definition of "diff --git", removing the "deleted file" or "new file" line
> makes the patch an invalid "git format diff". See the beginning of
> parse_git_header() where we say "we don't guess" and initialize both
> is_new and is_delete to false (and we flip them upon seeing "deleted file"
> and "new file", but never with "/dev/null").
Hmm. In that case, I think converting it to deletion is definitely
wrong; "diff --git" is about not guessing. So the only question is
whether it should be flagged as an error. I was somewhat worried that
you could produce a patch which would make apply complain by doing "git
diff /dev/null /your/file". But actually that already produces a "new
file" header (and the opposite produces a "deleted file" header).
So I think the patch below would notice both the new and deleted cases,
and is probably a good thing.
diff --git a/builtin-apply.c b/builtin-apply.c
index c8372a0..43a1535 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -673,8 +673,12 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
*/
static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
{
- if (!orig_name && !isnull)
+ if (!orig_name && !isnull) {
+ if (!memcmp(line, "/dev/null\n", 10))
+ die("git apply: bad git-diff - expected filename, got /dev/null on line %d", linenr);
return find_name(line, NULL, p_value, TERM_TAB);
+ }
+
if (orig_name) {
int len;
> I think some recent other SCMs produce what they claim to be "diff --git",
> but I don't know if they implement the format correctly enough. I am not
> worried about their implemention of binary patches (if they do not
> implement it correctly they will most likely get garbage), but do they get
> the abbreviated hash on the "index" line correctly? You can put garbage
> on the line and most of the time it would work but it will break "am -3"
> by breaking "apply --build-fake-ancestor".
>
> I just checked "hg diff --git"; at least it shows "deleted file".
Ugh. A whole new source of problems. :) I am not too interested in
seeking out and evaluating other SCM's implementations; I think we
should wait for people who actually use those systems to find
interoperability bugs, determine whether they are or are not simply bugs
in the other people's implementations, and then report the bug to us.
> > That would take some refactoring, though, as pulling the deletion hunk
> > out means we are re-ordering the headers. So right now if you did that
> > your ($head, @hunk) output would be something like:
> >
> > diff --git a/foo b/foo
> > index 257cc56..0000000
> > --- a/foo
> > +++ /dev/null
> > deleted file mode 100644
> > @@ -1 +0,0 @@
> > -foo
> >
> > which is pretty weird.
>
> I agree it is weird.
Note that we already do this for mode changes which also have a content
change. They look like:
diff --git a/foo b/foo
index 257cc56..19c6cc1
--- a/foo
+++ b/foo
old mode 100644
new mode 100755
Stage mode change [y,n,q,a,d,/,j,J,g,?]?
@@ -1 +1,2 @@
foo
+content
Stage this hunk [y,n,q,a,d,/,K,g,e,?]?
> Interesting. Does "add -p" (especially its [e]dit codepath) know enough
> about what it is doing? If so, it should be able to add "deleted file" on
> its own (and remove it when the result of editing and picking hunks makes
> the patch a non-deletion). For example, if you have a two-liner in the
> index and have deleted one line in the work tree, and run "add -p":
No, it doesn't know enough now. That would be part of the refactoring I
mentioned. I'm not sure how useful it is to support this. I can see
going from "I had hunk A, but I really wanted to tweak it to hunk B". I
can't think of a single time I've wanted "I deleted the entire file, but
I really wanted to keep 2 lines". And if I did, I would probably just:
git checkout file
$EDITOR file
git add -p file
> Perhaps the "add -i" at the end should offer, after noticing that the
> chosen and edited hunks will make the postimage an empty file, a chance
> for the user to say "I not only want to remove the contents from the path,
> but want to remove the path itself" in such a case?
>
> I dunno.
I would not say no to such a patch, but I really have no interest in
writing it myself.
-Peff
^ permalink raw reply related
* Re: [PATCH 0/3] Add a "fix" command to "rebase --interactive"
From: Junio C Hamano @ 2009-12-08 7:43 UTC (permalink / raw)
To: Nanako Shiraishi
Cc: Junio C Hamano, Johannes Schindelin, Matthieu Moy,
Michael J Gruber, Michael Haggerty, git
In-Reply-To: <20091208150102.6117@nanako3.lavabit.com>
Nanako Shiraishi <nanako3@lavabit.com> writes:
>> Hmph, did you forget to retitle the message, or keep in-body "Subject:"?
>
> Sorry. Yes I did. Please amend it to -
>
> Subject: rebase -i --autosquash: auto-squash commits
Ok.
^ permalink raw reply
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Jeff King @ 2009-12-08 7:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James Vega, git
In-Reply-To: <7v3a3lorge.fsf@alter.siamese.dyndns.org>
On Mon, Dec 07, 2009 at 11:28:01PM -0800, Junio C Hamano wrote:
> That might be something we may want to fix someday, when we find ourselves
> needing to add a feature to turn deletion into non-deletion or vice versa
> during "add -p" [e]dit, as I suspect that the "hunk editing" codepath does
> not keep track of what the user's patch is doing, to the point that it
> does not even know how many lines there are supposed to be in the
> resulting hunk that it asks "git apply" to recount. There is no way to
> add/delete "deleted file" line if the logic does not know what the patch
> is doing.
>
> But someday is not today. I think this six-liner is preferable.
OK, here it is with the test and an amended commit message. You could
almost do an [e]dit on this and delete the "deleted" line, but you have
no way of fixing up the "+++ /dev/null" line. For now, we have
disabled [e]dit entirely for non-content hunks, so at least you cannot
get yourself into trouble creating a broken patch. :)
-- >8 --
Subject: [PATCH] add-interactive: fix deletion of non-empty files
Commit 24ab81a fixed the deletion of empty files, but broke
deletion of non-empty files. The approach it took was to
factor out the "deleted" line from the patch header into its
own hunk, the same way we do for mode changes. However,
unlike mode changes, we only showed the special "delete this
file" hunk if there were no other hunks. Otherwise, the user
would annoyingly be presented with _two_ hunks: one for
deleting the file and one for deleting the content.
This meant that in the non-empty case, we forgot about the
deleted line entirely, and we submitted a bogus patch to
git-apply (with "/dev/null" as the destination file, but not
marked as a deletion).
Instead, this patch combines the file deletion hunk and the
content deletion hunk (if there is one) into a single
deletion hunk which is either staged or not.
Signed-off-by: Jeff King <peff@peff.net>
---
git-add--interactive.perl | 6 +++++-
t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..02e97b9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1217,7 +1217,11 @@ sub patch_update_file {
if (@{$mode->{TEXT}}) {
unshift @hunk, $mode;
}
- if (@{$deletion->{TEXT}} && !@hunk) {
+ if (@{$deletion->{TEXT}}) {
+ foreach my $hunk (@hunk) {
+ push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
+ push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
+ }
@hunk = ($deletion);
}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index aa5909b..0926b91 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '
'
cat >expected <<EOF
+diff --git a/non-empty b/non-empty
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/non-empty
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+test_expect_success 'deleting a non-empty file' '
+ git reset --hard &&
+ echo content >non-empty &&
+ git add non-empty &&
+ git commit -m non-empty &&
+ rm non-empty &&
+ echo y | git add -p non-empty &&
+ git diff --cached >diff &&
+ test_cmp expected diff
+'
+
+cat >expected <<EOF
diff --git a/empty b/empty
deleted file mode 100644
index e69de29..0000000
--
1.6.5.1.g24ab.dirty
^ permalink raw reply related
* Re: git-apply fails on creating a new file, with both -p and --directory specified
From: Junio C Hamano @ 2009-12-08 7:53 UTC (permalink / raw)
To: Jeff King; +Cc: James Vega, git
In-Reply-To: <20091208074935.GB12049@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> OK, here it is with the test and an amended commit message. You could
> almost do an [e]dit on this and delete the "deleted" line, but you have
> no way of fixing up the "+++ /dev/null" line. For now, we have
> disabled [e]dit entirely for non-content hunks, so at least you cannot
> get yourself into trouble creating a broken patch. :)
;-)
Thanks.
^ permalink raw reply
* Re: [PATCH] Add commit.status, --status, and --no-status
From: Jeff King @ 2009-12-08 7:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James P. Howard, II, git
In-Reply-To: <7vr5r6ndkz.fsf@alter.siamese.dyndns.org>
On Mon, Dec 07, 2009 at 11:13:00PM -0800, Junio C Hamano wrote:
> >> This commit provides support for commit.status, --status, and
> >> --no-status, which control whether or not the git status information
> >> is included in the commit message template when using an editor to
> >> prepare the commit message. It does not affect the effects of a
> >> user's commit.template settings.
> >
> > Thanks, this looks very cleanly done. The only complaint I would make is
> > that it should probably include a simple test case.
>
> Yes. Also I am a _bit_ worried about the name "status", as the longer
> term direction is to make "status" not "a preview of commit", may confuse
> people who do read Release Notes.
I thought about that, but what other name does it have? That text has
always been called "status", and we will continue to support that output
format as "git status" _and_ as "commit --dry-run". So I think
explaining it as "usually we stick the output of 'git status' into the
commit message, but this suppresses it" is not that hard (and that was
how I read the documentation in his patch).
The only trick is that it is not a vanilla "git status", but rather
"status after we have staged things for commit". But I think that is
fairly obvious since you are, after all, calling "commit".
But then again, I am probably way too deep in this topic to provide a
regular git user's perspective of what is obvious.
-Peff
^ permalink raw reply
* [PATCH v4 1/6] reset: add a few tests for "git reset --merge"
From: Christian Couder @ 2009-12-08 7:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow, Jakub Narebski, Paolo Bonzini, Johannes Sixt
In-Reply-To: <20091208075005.4475.26582.chriscool@tuxfamily.org>
Commit 9e8eceab ("Add 'merge' mode to 'git reset'", 2008-12-01),
added the --merge option to git reset, but there were no test cases
for it.
This was not a big problem because "git reset" was just forking and
execing "git read-tree", but this will change in a following patch.
So let's add a few test cases to make sure that there will be no
regression.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t7110-reset-merge.sh | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 94 insertions(+), 0 deletions(-)
create mode 100755 t/t7110-reset-merge.sh
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
new file mode 100755
index 0000000..8190da1
--- /dev/null
+++ b/t/t7110-reset-merge.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Christian Couder
+#
+
+test_description='Tests for "git reset --merge"'
+
+. ./test-lib.sh
+
+test_expect_success 'creating initial files' '
+ echo "line 1" >> file1 &&
+ echo "line 2" >> file1 &&
+ echo "line 3" >> file1 &&
+ cp file1 file2 &&
+ git add file1 file2 &&
+ test_tick &&
+ git commit -m "Initial commit"
+'
+
+test_expect_success 'reset --merge is ok with changes in file it does not touch' '
+ echo "line 4" >> file1 &&
+ echo "line 4" >> file2 &&
+ test_tick &&
+ git commit -m "add line 4" file1 &&
+ git reset --merge HEAD^ &&
+ ! grep 4 file1 &&
+ grep 4 file2 &&
+ git reset --merge HEAD@{1} &&
+ grep 4 file1 &&
+ grep 4 file2
+'
+
+test_expect_success 'reset --merge discards changes added to index (1)' '
+ echo "line 5" >> file1 &&
+ git add file1 &&
+ git reset --merge HEAD^ &&
+ ! grep 4 file1 &&
+ ! grep 5 file1 &&
+ grep 4 file2 &&
+ echo "line 5" >> file2 &&
+ git add file2 &&
+ git reset --merge HEAD@{1} &&
+ ! grep 4 file2 &&
+ ! grep 5 file1 &&
+ grep 4 file1
+'
+
+test_expect_success 'reset --merge discards changes added to index (2)' '
+ echo "line 4" >> file2 &&
+ git add file2 &&
+ git reset --merge HEAD^ &&
+ ! grep 4 file2 &&
+ git reset --merge HEAD@{1} &&
+ ! grep 4 file2 &&
+ grep 4 file1
+'
+
+test_expect_success 'reset --merge fails with changes in file it touches' '
+ echo "line 5" >> file1 &&
+ test_tick &&
+ git commit -m "add line 5" file1 &&
+ sed -e "s/line 1/changed line 1/" <file1 >file3 &&
+ mv file3 file1 &&
+ test_must_fail git reset --merge HEAD^ 2>err.log &&
+ grep file1 err.log | grep "not uptodate" &&
+ git reset --hard HEAD^
+'
+
+test_expect_success 'setup 2 different branches' '
+ git branch branch1 &&
+ git branch branch2 &&
+ git checkout branch1 &&
+ echo "line 5 in branch1" >> file1 &&
+ test_tick &&
+ git commit -a -m "change in branch1" &&
+ git checkout branch2 &&
+ echo "line 5 in branch2" >> file1 &&
+ test_tick &&
+ git commit -a -m "change in branch2"
+'
+
+test_expect_success '"reset --merge HEAD^" fails with pending merge' '
+ test_must_fail git merge branch1 &&
+ test_must_fail git reset --merge HEAD^ &&
+ git reset --hard HEAD
+'
+
+test_expect_success '"reset --merge HEAD" fails with pending merge' '
+ test_must_fail git merge branch1 &&
+ test_must_fail git reset --merge HEAD &&
+ git reset --hard HEAD
+'
+
+test_done
--
1.6.5.1.gaf97d
^ permalink raw reply related
* [PATCH v4 2/6] reset: use "unpack_trees()" directly instead of "git read-tree"
From: Christian Couder @ 2009-12-08 7:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow, Jakub Narebski, Paolo Bonzini, Johannes Sixt
In-Reply-To: <20091208075005.4475.26582.chriscool@tuxfamily.org>
From: Stephan Beyer <s-beyer@gmx.net>
This patch makes "reset_index_file()" call "unpack_trees()" directly
instead of forking and execing "git read-tree". So the code is more
efficient.
And it's also easier to see which unpack_tree() options will be used,
as we don't need to follow "git read-tree"'s command line parsing
which is quite complex.
As Daniel Barkalow found, there is a difference between this new
version and the old one. The old version gives an error for
"git reset --merge" with unmerged entries and the new version does
not. But this can be seen as a bug fix, because "--merge" was the
only "git reset" option with this behavior and this behavior was
not documented.
In fact there is still an error with unmerge entries if we reset
the unmerge entries to the same state as HEAD. So the bug is not
completely fixed.
The code comes from the sequencer GSoC project:
git://repo.or.cz/git/sbeyer.git
(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-reset.c | 51 +++++++++++++++++++++++++++++++++++++----------
t/t7110-reset-merge.sh | 8 ++++--
2 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index 73e6022..ddb81f3 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -18,6 +18,8 @@
#include "tree.h"
#include "branch.h"
#include "parse-options.h"
+#include "unpack-trees.h"
+#include "cache-tree.h"
static const char * const git_reset_usage[] = {
"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
@@ -52,29 +54,56 @@ static inline int is_merge(void)
return !access(git_path("MERGE_HEAD"), F_OK);
}
+static int parse_and_init_tree_desc(const unsigned char *sha1,
+ struct tree_desc *desc)
+{
+ struct tree *tree = parse_tree_indirect(sha1);
+ if (!tree)
+ return 1;
+ init_tree_desc(desc, tree->buffer, tree->size);
+ return 0;
+}
+
static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
{
- int i = 0;
- const char *args[6];
+ int nr = 1;
+ int newfd;
+ struct tree_desc desc[2];
+ struct unpack_trees_options opts;
+ struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- args[i++] = "read-tree";
+ memset(&opts, 0, sizeof(opts));
+ opts.head_idx = 1;
+ opts.src_index = &the_index;
+ opts.dst_index = &the_index;
+ opts.fn = oneway_merge;
+ opts.merge = 1;
if (!quiet)
- args[i++] = "-v";
+ opts.verbose_update = 1;
switch (reset_type) {
case MERGE:
- args[i++] = "-u";
- args[i++] = "-m";
+ opts.update = 1;
break;
case HARD:
- args[i++] = "-u";
+ opts.update = 1;
/* fallthrough */
default:
- args[i++] = "--reset";
+ opts.reset = 1;
}
- args[i++] = sha1_to_hex(sha1);
- args[i] = NULL;
- return run_command_v_opt(args, RUN_GIT_CMD);
+ newfd = hold_locked_index(lock, 1);
+
+ read_cache_unmerged();
+
+ if (parse_and_init_tree_desc(sha1, desc + nr - 1))
+ return error("Failed to find tree of %s.", sha1_to_hex(sha1));
+ if (unpack_trees(nr, desc, &opts))
+ return -1;
+ if (write_cache(newfd, active_cache, active_nr) ||
+ commit_locked_index(lock))
+ return error("Could not write new index file.");
+
+ return 0;
}
static void print_new_head_line(struct commit *commit)
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 8190da1..6afaf73 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -79,10 +79,12 @@ test_expect_success 'setup 2 different branches' '
git commit -a -m "change in branch2"
'
-test_expect_success '"reset --merge HEAD^" fails with pending merge' '
+test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
test_must_fail git merge branch1 &&
- test_must_fail git reset --merge HEAD^ &&
- git reset --hard HEAD
+ git reset --merge HEAD^ &&
+ test -z "$(git diff --cached)" &&
+ test -n "$(git diff)" &&
+ git reset --hard HEAD@{1}
'
test_expect_success '"reset --merge HEAD" fails with pending merge' '
--
1.6.5.1.gaf97d
^ permalink raw reply related
* [PATCH v4 0/6] "git reset --merge" related improvements
From: Christian Couder @ 2009-12-08 7:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow, Jakub Narebski, Paolo Bonzini, Johannes Sixt
This is a reroll of a previous series from last september:
http://thread.gmane.org/gmane.comp.version-control.git/128706/focus=128707
The changes in this reroll are the following:
- new option was renamed "--keep-local-changes" instead of "--merge-safe",
- some test cases have been added,
- a bug was fixed (head_sha1 is now "unsigned char head_sha1[20]"),
- I took ownership of the third patch,
- some commit messages were improved,
- there are 2 new documentation patches at the end,
- the last documentation patch adds some tables about what all the reset
options are doing in the different cases.
The new option name is "--keep-local-changes" because that's what
Junio used in the last email of the previous discussion, but my
opinion is that it is a bit long and so I'd like to rename it "--keep"
or another such short name.
Christian Couder (5):
reset: add a few tests for "git reset --merge"
reset: add option "--keep-local-changes" to "git reset"
reset: add test cases for "--keep-local-changes" option
Documentation: reset: describe new "--keep-local-changes" option
Documentation: reset: add some tables to describe the different
options
Stephan Beyer (1):
reset: use "unpack_trees()" directly instead of "git read-tree"
Documentation/git-reset.txt | 80 ++++++++++++++++++++++-
builtin-reset.c | 81 ++++++++++++++++++-----
t/t7110-reset-merge.sh | 156 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 300 insertions(+), 17 deletions(-)
create mode 100755 t/t7110-reset-merge.sh
^ permalink raw reply
* [PATCH v4 3/6] reset: add option "--keep-local-changes" to "git reset"
From: Christian Couder @ 2009-12-08 7:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow, Jakub Narebski, Paolo Bonzini, Johannes Sixt
In-Reply-To: <20091208075005.4475.26582.chriscool@tuxfamily.org>
The purpose of this new option is to discard some of the
last commits but to keep current changes in the work tree
and the index.
The table below shows what happens when running
"git reset --option target" to reset the HEAD to another
commit (as a special case "target" could be the same as
HEAD) in the cases where "--merge" and
"--keep-local-changes" (abreviated --k-l-c) behave
differently.
working index HEAD target working index HEAD
----------------------------------------------------
B B A A --k-l-c B A A
--merge A A A
B B A C --k-l-c (disallowed)
--merge C C C
In this table, A, B and C are some different states of
a file. For example the first line of the table means
that if a file is in state B in the working tree and
the index, and in a different state A in HEAD and in
the target, then "git reset --keep-local-changes target"
will put the file in state B in the working tree and in
state A in the index and HEAD.
So as can be seen in the table, "--merge" discards changes
in the index, while "--keep-local-changes" does not. So
if one wants to avoid using "git stash" before and after
using "git reset" to save current changes, it is better to
use "--keep-local-changes" rather than "--merge".
The following table shows what happens on unmerged entries:
working index HEAD target working index HEAD
----------------------------------------------------
X U A B --k-l-c X B B
--merge X B B
X U A A --k-l-c X A A
--merge (disallowed)
In this table X can be any state and U means an unmerged
entry.
A following patch will add some test cases for
"--keep-local-changes", where the differences between
"--merge" and "--keep-local-changes" can also be seen.
The "--keep-local-changes" option is implemented by doing
a 2 way merge between HEAD and the reset target, and if
this succeeds by doing a mixed reset to the target.
The code comes from the sequencer GSoC project, where
such an option was developed by Stephan Beyer:
git://repo.or.cz/git/sbeyer.git
(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
But in the sequencer project the "reset" flag was set
in the "struct unpack_trees_options" passed to
"unpack_trees()". With this flag the changes in the
working tree were discarded if the file was different
between HEAD and the reset target.
Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-reset.c | 30 +++++++++++++++++++++++++-----
1 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index ddb81f3..3cbc4fd 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -22,13 +22,15 @@
#include "cache-tree.h"
static const char * const git_reset_usage[] = {
- "git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
+ "git reset [--mixed | --soft | --hard | --merge | --keep-local-changes] [-q] [<commit>]",
"git reset [--mixed] <commit> [--] <paths>...",
NULL
};
-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
-static const char *reset_type_names[] = { "mixed", "soft", "hard", "merge", NULL };
+enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP_LOCAL_CHANGES, NONE };
+static const char *reset_type_names[] = {
+ "mixed", "soft", "hard", "merge", "keep_local_changes", NULL
+};
static char *args_to_str(const char **argv)
{
@@ -81,6 +83,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
if (!quiet)
opts.verbose_update = 1;
switch (reset_type) {
+ case KEEP_LOCAL_CHANGES:
case MERGE:
opts.update = 1;
break;
@@ -95,6 +98,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
read_cache_unmerged();
+ if (reset_type == KEEP_LOCAL_CHANGES) {
+ unsigned char head_sha1[20];
+ if (get_sha1("HEAD", head_sha1))
+ return error("You do not have a valid HEAD.");
+ if (parse_and_init_tree_desc(head_sha1, desc))
+ return error("Failed to find tree of HEAD.");
+ nr++;
+ opts.fn = twoway_merge;
+ }
+
if (parse_and_init_tree_desc(sha1, desc + nr - 1))
return error("Failed to find tree of %s.", sha1_to_hex(sha1));
if (unpack_trees(nr, desc, &opts))
@@ -238,6 +251,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
"reset HEAD, index and working tree", HARD),
OPT_SET_INT(0, "merge", &reset_type,
"reset HEAD, index and working tree", MERGE),
+ OPT_SET_INT(0, "keep-local-changes", &reset_type,
+ "reset HEAD but keep local changes",
+ KEEP_LOCAL_CHANGES),
OPT_BOOLEAN('q', NULL, &quiet,
"disable showing new HEAD in hard reset and progress message"),
OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
@@ -324,9 +340,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type == SOFT) {
if (is_merge() || read_cache() < 0 || unmerged_cache())
die("Cannot do a soft reset in the middle of a merge.");
+ } else {
+ int err = reset_index_file(sha1, reset_type, quiet);
+ if (reset_type == KEEP_LOCAL_CHANGES)
+ err = err || reset_index_file(sha1, MIXED, quiet);
+ if (err)
+ die("Could not reset index file to revision '%s'.", rev);
}
- else if (reset_index_file(sha1, reset_type, quiet))
- die("Could not reset index file to revision '%s'.", rev);
/* Any resets update HEAD to the head being switched to,
* saving the previous head in ORIG_HEAD before. */
--
1.6.5.1.gaf97d
^ permalink raw reply related
* [PATCH v4 4/6] reset: add test cases for "--keep-local-changes" option
From: Christian Couder @ 2009-12-08 7:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow, Jakub Narebski, Paolo Bonzini, Johannes Sixt
In-Reply-To: <20091208075005.4475.26582.chriscool@tuxfamily.org>
This shows that with the "--keep-local-changes" option, changes that
are both in the work tree and the index are kept in the work tree
after the reset (but discarded in the index). As with the "--merge"
option, changes that are in both the work tree and the index are
discarded after the reset.
In the case of unmerged entries, we can see that
"git reset --merge HEAD" is disallowed while it works using
"--keep-local-changes" instead.
And this shows that otherwise "--merge" and "--keep-local-changes"
have the same behavior.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/t7110-reset-merge.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 61 insertions(+), 1 deletions(-)
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 6afaf73..c8f72cd 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2009 Christian Couder
#
-test_description='Tests for "git reset --merge"'
+test_description='Tests for "git reset" with --merge and --keep-local-changes'
. ./test-lib.sh
@@ -30,6 +30,20 @@ test_expect_success 'reset --merge is ok with changes in file it does not touch'
grep 4 file2
'
+test_expect_success 'reset --keep-local-changes is ok with changes in file it does not touch' '
+ git reset --hard HEAD^ &&
+ echo "line 4" >> file1 &&
+ echo "line 4" >> file2 &&
+ test_tick &&
+ git commit -m "add line 4" file1 &&
+ git reset --keep-local-changes HEAD^ &&
+ ! grep 4 file1 &&
+ grep 4 file2 &&
+ git reset --keep-local-changes HEAD@{1} &&
+ grep 4 file1 &&
+ grep 4 file2
+'
+
test_expect_success 'reset --merge discards changes added to index (1)' '
echo "line 5" >> file1 &&
git add file1 &&
@@ -55,6 +69,25 @@ test_expect_success 'reset --merge discards changes added to index (2)' '
grep 4 file1
'
+test_expect_success 'reset --keep-local-changes fails with changes in index in files it touches' '
+ echo "line 4" >> file2 &&
+ echo "line 5" >> file1 &&
+ git add file1 &&
+ test_must_fail git reset --keep-local-changes HEAD^ &&
+ git reset --hard HEAD
+'
+
+test_expect_success 'reset --keep-local-changes keeps changes it does not touch' '
+ echo "line 4" >> file2 &&
+ git add file2 &&
+ git reset --keep-local-changes HEAD^ &&
+ grep 4 file2 &&
+ git reset --keep-local-changes HEAD@{1} &&
+ grep 4 file2 &&
+ grep 4 file1 &&
+ git reset --hard HEAD
+'
+
test_expect_success 'reset --merge fails with changes in file it touches' '
echo "line 5" >> file1 &&
test_tick &&
@@ -66,6 +99,17 @@ test_expect_success 'reset --merge fails with changes in file it touches' '
git reset --hard HEAD^
'
+test_expect_success 'reset --keep-local-changes fails with changes in file it touches' '
+ echo "line 5" >> file1 &&
+ test_tick &&
+ git commit -m "add line 5" file1 &&
+ sed -e "s/line 1/changed line 1/" <file1 >file3 &&
+ mv file3 file1 &&
+ test_must_fail git reset --keep-local-changes HEAD^ 2>err.log &&
+ grep file1 err.log | grep "not uptodate" &&
+ git reset --hard HEAD^
+'
+
test_expect_success 'setup 2 different branches' '
git branch branch1 &&
git branch branch2 &&
@@ -87,10 +131,26 @@ test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
git reset --hard HEAD@{1}
'
+test_expect_success '"reset --keep-local-changes HEAD^" is ok with pending merge' '
+ test_must_fail git merge branch1 &&
+ git reset --keep-local-changes HEAD^ &&
+ test -z "$(git diff --cached)" &&
+ test -n "$(git diff)" &&
+ git reset --hard HEAD@{1}
+'
+
test_expect_success '"reset --merge HEAD" fails with pending merge' '
test_must_fail git merge branch1 &&
test_must_fail git reset --merge HEAD &&
git reset --hard HEAD
'
+test_expect_success '"reset --keep-local-changes HEAD" is ok with pending merge' '
+ test_must_fail git merge branch1 &&
+ git reset --keep-local-changes HEAD &&
+ test -z "$(git diff --cached)" &&
+ test -n "$(git diff)" &&
+ git reset --hard HEAD
+'
+
test_done
--
1.6.5.1.gaf97d
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox