* Re: [PATCH 7/7] revert: stop creating and removing sequencer-old directory
From: Ramkumar Ramachandra @ 2011-12-14 16:10 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210130612.GH22035@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> That's the end.
Apart from the few minor details, I'm happy with the series overall.
> I hope the patches provided some amusement, and
> advice towards making them more useful would be welcome.
Oh, yes. There were quite a few :facepalm: moments in there for me ;)
Thanks for the enjoyable read.
-- Ram
^ permalink raw reply
* Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))
From: Thomas Rast @ 2011-12-14 16:17 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Jeff King,
Nguyễn Thái Ngọc Duy, Michael Haggerty
In-Reply-To: <20111214070916.GA14954@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Junio C Hamano wrote:
> > Will merge to 'next' after taking another look.
>
> The middle commit looks good. The bottom commit could be improved as
> discussed at [1], but I guess that can happen in-tree.
>
> However, the top commit ("test test-terminal's sanity") still does not
> seem right to me.
I wasn't under the impression that we were done with this, either :-)
> It makes the same test run three times. Probably I should send an
> alternate patch to get that sanity-check to run once, but I am also
> not convinced the sanity-check is needed at all --- wouldn't any test
> that is relying on output from test_terminal act as a sanity check for
> it already?
It didn't. Or more precisely, Michael Haggerty ran into the behavior
of
git rev-parse ... | while read sha; do git checkout $sha; make test; done
couldn't make any sense of it, and reported it on IRC. So in some
sense, it took infrequent circumstances and two developers' time; next
time around I'd prefer it to be detected automatically.
> As an aside, I also still believe that running "git shortlog" without
> explicitly passing "HEAD" when testing how it reacts to [core] pager
> configuration was a bug and a distraction, hence the patch at [2].
Why not. Some other test should verify how shortlog reacts to the
tty-ness of stdin, but that's yet another direction.
> I also find Jeff's patch [3] appealing.
Me too, though wonder whether feeding a file full of garbage wouldn't
be better, so as to trip up commands that try to read only from a
non-tty stdin.
> [1] http://thread.gmane.org/gmane.comp.version-control.git/186923/focus=186944
> [2] http://article.gmane.org/gmane.comp.version-control.git/186932
> [3] http://article.gmane.org/gmane.comp.version-control.git/186936
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence
From: Jonathan Nieder @ 2011-12-14 16:21 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <CALkWK0mt03SSNT-svUO1wHdq5OpM=0xQO3FHkSGGEDuW-jUEXA@mail.gmail.com>
(out of order for convenience)
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Suggested-by: Johannes Sixt <j6t@kdbg.org>
>
> Could you link to the corresponding thread with Johannes?
No, I prefer not to. If I did a good job, the commit message would
explain enough already, and in exceptional cases, the interested
reader can look up the mailing list message the commit comes from and
walk upthread, no?
> Cute feature, although I don't ever recall needing it personally. Why
> does this relatively esoteric "feature" belong along with the other
> "maintenance patches" in jn/maint-sequencer-fixes?
Read ahead in the series, or read the cover letter. :)
> What I'm really interested in seeing is
> how you persist opts for "cherry-pick --continue" when a single-commit
> pick fails: in other words, how you manage to get " --continue of
> single-pick respects -x" to pass.
That's a good question. I did the lazy thing and let the existing
"git cherry-pick" logic take care of it (it writes MERGE_MSG).
>> + struct commit *cmit;
>> + if (prepare_revision_walk(opts->revs))
>> + die(_("revision walk setup failed"));
>> + cmit = get_revision(opts->revs);
>> + if (!cmit || get_revision(opts->revs))
>> + die("BUG: expected exactly one commit from walk");
>> + return single_pick(cmit, opts);
>> + }
>
> I'd have expected you to reuse prepare_revs().
Why? The purposes do not overlap much.
>> + if (opts->revs->cmdline.nr == 1 &&
>> + opts->revs->cmdline.rev->whence == REV_CMD_REV &&
>> + opts->revs->no_walk &&
>> + !opts->revs->cmdline.rev->flags) {
>
> Yuck, seriously.
> 1. I'd have expected you to check opts->revs->commits, not
> opts->revs->cmdline.nr. Okay, you're using the cmdline because the
> revision walk hasn't happened yet.
It would have been easy to do a revision walk and count and I'm using
the cmdline instead deliberately --- the goal really is "anything more
complicated than a simple rev on the command line should trip the
multi-pick logic".
I admit though that I'm not too familiar with the new cmdline_info
API. I'd welcome a simpler expression with the same effect.
Also, I probably should have included a test that does some
git cherry-pick picked^..picked
thing and verifies that this is treated as a multi-pick. And
documented this. :)
Thanks for pointing out the questionable bits. I am tempted to reroll
to put this after patch 6/7, which would make it possible to use "git
reset --merge" in the commit message for a more natural explanation.
That would also provide an opportunity to reuse some text from [1],
which in hindsight seems to have explained some aspects of each patch
a little more clearly.
Thanks, and hoping that clarifies a little,
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/185716/focus=186811
^ permalink raw reply
* Re: Git difftool / mergetool on directory tree
From: Thomas Rast @ 2011-12-14 16:25 UTC (permalink / raw)
To: Daniele Segato; +Cc: Git Mailing List
In-Reply-To: <4EE8618E.7020902@gmail.com>
Daniele Segato wrote:
> Hi,
>
> many diff / merge tool around have the ability to compare a directory
> tree (meld is one, but there are many).
>
> Is there a way to start a difftool or a mergetool on a folder instead of
> the single file?
>
> It would be an handsome feature to git.
>
> I googled a little before popping out this question and I only found
> suggestion on how to open "many" file at once instead of opening them
> serially but that's not the same thing not as powerful as directory
> comparison.
Maybe this helps:
http://thread.gmane.org/gmane.comp.version-control.git/144839
It was never applied however.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming
From: Jonathan Nieder @ 2011-12-14 16:48 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <CALkWK0miBzT4BXRDYOhz8JqF2jeyz1L3=pwaGKVm654oHtQbtQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Sounds good. I remember my implementation being quite complicated;
> let's see how you've done this.
I have to confess that I don't remember the implementation you are
referring to. Maybe I could have taken inspiration from it.
The rest of this message is about tests.
[...]
> Jonathan Nieder wrote:
>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>> index 2c4c1c85..4d1883b7 100755
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -2,6 +2,7 @@
>>
>> test_description='Test cherry-pick continuation features
>>
>> + + conflicting: rewrites unrelated to conflicting
>> + yetanotherpick: rewrites foo to e
>> + anotherpick: rewrites foo to d
>> + picked: rewrites foo to c
>
> Note to self: this list of commits is becoming quite unwieldy. We
> should probably refactor these sometime.
To clarify, "conflicting" is sitting on a separate branch from the
rest of the commits. This --help text uses "git show-branch"-style
output, which is perhaps out of fashion but compact and used by some
existing tests.
>> @@ -27,6 +28,7 @@ test_cmp_rev () {
>> }
>>
>> test_expect_success setup '
>> + git config advice.detachedhead false
>> echo unrelated >unrelated &&
>> git add unrelated &&
>> test_commit initial foo a &&
>
> Huh, why are you moving this line up? Oh, right: there are
> "test_commit" statements in the setup- good catch.
Nah, it's for the pristine_detach, not the test_commit.
I did miss an &&, though. Not enough to justify rerolling on its own
but seems worth fixing if resending anyway.
>> @@ -35,8 +37,8 @@ test_expect_success setup '
>> test_commit picked foo c &&
>> test_commit anotherpick foo d &&
>> test_commit yetanotherpick foo e &&
>> - git config advice.detachedhead false
>> -
>> + pristine_detach initial &&
>> + test_commit conflicting unrelated
>> '
>
> Looks fishy- I wonder why you're doing this. Let's read ahead and find out.
Do you mean that you'd prefer this "conflicting" commit not to be
part of the setup shared between tests?
[...]
>> +test_expect_success '--continue of single revert' '
>> + pristine_detach initial &&
>> + echo resolved >expect &&
>> + echo "Revert \"picked\"" >expect.msg &&
>> + test_must_fail git revert picked &&
>> + echo resolved >foo &&
>> + git add foo &&
>> + git cherry-pick --continue &&
>
> Huh? You're continuing a "git revert" with a a "git cherry-pick
> --continue"?
Yep, works fine.
[...]
> 1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
> up the documentation to find this:
Yeah, that documentation sucks. I'll keep this message marked as a
reminder to look at it.
Just like "git show" is the porcelain command to show a commit, "git
diff-tree" is the corresponding plumbing.
[...]
>> +test_expect_success '--continue after resolving conflicts' '
[...]
> Unchanged from the original: I suspect you've moved the generation of
> expectation messages up to produce a clean diff.
It's just a new test. If rerolling, I'll make it imitate the style of
the existing test following it better.
[...]
>> +test_expect_success '--continue asks for help after resolving patch to nil' '
>> + pristine_detach conflicting &&
>> + test_must_fail git cherry-pick initial..picked &&
>> +
>> + test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
>> + git checkout HEAD -- unrelated &&
>> + test_must_fail git cherry-pick --continue 2>msg &&
>> + test_i18ngrep "The previous cherry-pick is now empty" msg
>> +'
>
> I thought it was a bad idea to grep for specific output messages,
> because of their volatile nature?
This test is about --continue asking for help instead of succeeding or
failing in some uncontrolled way, so it seemed useful to check that
the message actually pertains to that.
> Remind me what this test has to do
> with the rest of your patch?
With this change in how --continue works, I wanted to make sure the
semantics that were not supposed to be changed were still intact.
[...]
>> +test_expect_failure 'follow advice and skip nil patch' '
[...]
> Again, what does this test have to do with the rest of your patch?
Likewise.
[...]
>> +test_expect_success '--continue of single-pick respects -x' '
[...]
> I'd have liked s/respects -x/respects opts/ here for symmetry with the
> previous test.
Maybe the previous one should say "respects -x".
I am not sure what it would mean for --continue of a single-pick to
respect --strategy, for example.
>> +test_expect_success '--continue respects -x in first commit in multi-pick' '
[...]
> Can you explain why "first commit in a multi-pick" is a special case?
I guess you mean "how does this differ from the existing '--continue
respects opts' test?".
Good question. In the existing "--continue respects opts" test, we
explicitly run "git commit" before "git cherry-pick --continue". This
test checks that running "git cherry-pick --continue" without
commiting first does not cause the commit message to be clobbered.
[...]
>> @@ -306,6 +413,32 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
>> grep "Signed-off-by:" anotherpick_msg
>> '
>>
>> +test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
[...]
> Unrelated.
[...]
>> +test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
There was no implicit commit of resolution before this patch, so how
can it be unrelated?
[...]
> Thanks for working on this.
Thanks for your attention to detail.
Sincerely,
Jonathan
^ permalink raw reply
* [PATCH 00/10] Re-roll rr/revert-cherry-pick v3
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <CALkWK0kbV2WFfGVrA9m_Uwd4J8+U9Yde9Wxb-OZE9Y8K+Ta_4A@mail.gmail.com>
Hi,
There are 10 parts in this iteration. The main changes are:
- "t3502, t3510: clarify cherry-pick -m failure" and "revert: move
replay_action, replay_subcommand to header" are new. They've been
made separate parts for clarity.
- Since jn/maint-sequencer-fixes contains "revert: pass around
rev-list args in already-parsed form", I've dropped it from this
series. Except for the tests, I've checked that it doesn't conflict
with the topic.
- Rebased on new `master`.
- Other changes are fairly minor, and mainly arise as responses to
Jonathan's reviews.
Thanks.
-- Ram
Ramkumar Ramachandra (10):
revert: free msg in format_todo()
revert: make commit subjects in insn sheet optional
revert: tolerate extra spaces, tabs in insn sheet
revert: simplify getting commit subject in format_todo()
t3510 (cherry-pick-sequencer): use exit status
t3502, t3510: clarify cherry-pick -m failure
t3510 (cherry-pick-sequencer): remove malformed sheet 2
revert: move replay_action, replay_subcommand to header
revert: allow mixed pick and revert instructions
revert: report fine-grained error messages from insn parser
builtin/revert.c | 194 ++++++++++++++++++++-------------------
sequencer.h | 18 ++++
t/t3502-cherry-pick-merge.sh | 2 +-
t/t3510-cherry-pick-sequence.sh | 142 +++++++++++++++++++++++------
4 files changed, 231 insertions(+), 125 deletions(-)
--
1.7.4.1
^ permalink raw reply
* [PATCH 01/10] revert: free msg in format_todo()
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit. Fix this using
free_message().
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
if (get_message(cur->item, &msg))
return error(_("Cannot get commit message for %s"), sha1_abbrev);
strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+ free_message(&msg);
}
return 0;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 02/10] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
Change the instruction sheet format subtly so that the subject of the
commit message that follows the object name is optional. As a result,
an instruction sheet like this is now perfectly valid:
pick 35b0426
pick fbd5bbcbc2e
pick 7362160f
While at it, also fix a bug introduced by 5a5d80f4 (revert: Introduce
--continue to continue the operation, 2011-08-04) that failed to read
lines that are too long to fit on the commit-id-shaped buffer we
currently use; eliminate the need for the buffer altogether. In
addition to literal SHA-1 hexes, you can now safely use expressions
like the following in the instruction sheet:
featurebranch~4
rr/revert-cherry-pick-continue^2~12@{12 days ago}
[jc: simplify parsing]
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 37 ++++++++++++++++---------------------
t/t3510-cherry-pick-sequence.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
return 0;
}
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
{
unsigned char commit_sha1[20];
- char sha1_abbrev[40];
enum replay_action action;
- int insn_len = 0;
- char *p, *q;
+ char *end_of_object_name;
+ int saved, status;
- if (!prefixcmp(start, "pick ")) {
+ if (!prefixcmp(bol, "pick ")) {
action = CHERRY_PICK;
- insn_len = strlen("pick");
- p = start + insn_len + 1;
- } else if (!prefixcmp(start, "revert ")) {
+ bol += strlen("pick ");
+ } else if (!prefixcmp(bol, "revert ")) {
action = REVERT;
- insn_len = strlen("revert");
- p = start + insn_len + 1;
+ bol += strlen("revert ");
} else
return NULL;
- q = strchr(p, ' ');
- if (!q)
- return NULL;
- q++;
-
- strlcpy(sha1_abbrev, p, q - p);
+ end_of_object_name = bol + strcspn(bol, " \n");
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ status = get_sha1(bol, commit_sha1);
+ *end_of_object_name = saved;
/*
* Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
return NULL;
}
- if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+ if (status < 0)
return NULL;
return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
int i;
for (i = 1; *p; i++) {
- commit = parse_insn_line(p, opts);
+ char *eol = strchrnul(p, '\n');
+ commit = parse_insn_line(p, eol, opts);
if (!commit)
return error(_("Could not parse line %d."), i);
next = commit_list_append(commit, next);
- p = strchrnul(p, '\n');
- if (*p)
- p++;
+ p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features
. ./test-lib.sh
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
pristine_detach () {
git cherry-pick --quit &&
git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '
test_must_fail git cherry-pick --continue
'
+test_expect_success 'malformed instruction sheet 3' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "resolved" >foo &&
+ git add foo &&
+ git commit &&
+ sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ test_must_fail git cherry-pick --continue
+'
+
+test_expect_success 'commit descriptions in insn sheet are optional' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ git rev-list HEAD >commits &&
+ test_line_count = 4 commits
+'
+
test_done
--
1.7.4.1
^ permalink raw reply related
* [PATCH 03/10] revert: tolerate extra spaces, tabs in insn sheet
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
Tolerate extra spaces and tabs as part of the the field separator in
'.git/sequencer/todo', for people with fat fingers.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 18 ++++++++++++------
t/t3510-cherry-pick-sequence.sh | 11 +++++++++++
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..be0686d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -716,18 +716,24 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
unsigned char commit_sha1[20];
enum replay_action action;
char *end_of_object_name;
- int saved, status;
+ int saved, status, padding;
- if (!prefixcmp(bol, "pick ")) {
+ if (!prefixcmp(bol, "pick")) {
action = CHERRY_PICK;
- bol += strlen("pick ");
- } else if (!prefixcmp(bol, "revert ")) {
+ bol += strlen("pick");
+ } else if (!prefixcmp(bol, "revert")) {
action = REVERT;
- bol += strlen("revert ");
+ bol += strlen("revert");
} else
return NULL;
- end_of_object_name = bol + strcspn(bol, " \n");
+ /* Eat up extra spaces/ tabs before object name */
+ padding = strspn(bol, " \t");
+ if (!padding)
+ return NULL;
+ bol += padding;
+
+ end_of_object_name = bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
*end_of_object_name = '\0';
status = get_sha1(bol, commit_sha1);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6390f2a..781c5ac 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -342,6 +342,17 @@ test_expect_success 'malformed instruction sheet 3' '
test_must_fail git cherry-pick --continue
'
+test_expect_success 'instruction sheet, fat-fingers version' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ sed "s/pick \([0-9a-f]*\)/pick \1 /" .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ git cherry-pick --continue
+'
+
test_expect_success 'commit descriptions in insn sheet are optional' '
pristine_detach initial &&
test_must_fail git cherry-pick base..anotherpick &&
--
1.7.4.1
^ permalink raw reply related
* [PATCH 04/10] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
format_todo() calls get_message(), but uses only the subject line of
the commit message. As a minor optimization, save work and
unnecessary memory allocations by using find_commit_subject() instead.
Also, remove the unnecessary check on cur->item->buffer: the
lookup_commit_reference() call in parse_insn_line() has already made
sure of this.
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index be0686d..0a4b215 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
struct replay_opts *opts)
{
struct commit_list *cur = NULL;
- struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
const char *sha1_abbrev = NULL;
const char *action_str = opts->action == REVERT ? "revert" : "pick";
+ const char *subject;
+ int subject_len;
for (cur = todo_list; cur; cur = cur->next) {
sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- if (get_message(cur->item, &msg))
- return error(_("Cannot get commit message for %s"), sha1_abbrev);
- strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
- free_message(&msg);
+ subject_len = find_commit_subject(cur->item->buffer, &subject);
+ strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+ subject_len, subject);
}
return 0;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 05/10] t3510 (cherry-pick-sequencer): use exit status
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
All the tests asserting failure use 'test_must_fail', which simply
checks for a non-zero exit status, potentially hiding underlying bugs.
So, replace instances of 'test_must_fail' with 'test_expect_code' to
check the exit status explicitly, where appropriate.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++++-------------------
1 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 781c5ac..b6e822e 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -44,7 +44,7 @@ test_expect_success setup '
test_expect_success 'cherry-pick persists data on failure' '
pristine_detach initial &&
- test_must_fail git cherry-pick -s base..anotherpick &&
+ test_expect_code 1 git cherry-pick -s base..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
test_path_is_file .git/sequencer/todo &&
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
test_expect_success 'cherry-pick persists opts correctly' '
pristine_detach initial &&
- test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+ test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
test_path_is_file .git/sequencer/todo &&
@@ -93,7 +93,7 @@ test_expect_success '--abort requires cherry-pick in progress' '
test_expect_success '--quit cleans up sequencer state' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..picked &&
+ test_expect_code 1 git cherry-pick base..picked &&
git cherry-pick --quit &&
test_path_is_missing .git/sequencer
'
@@ -107,7 +107,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
:000000 100644 OBJID OBJID A foo
:000000 100644 OBJID OBJID A unrelated
EOF
- test_must_fail git cherry-pick base..picked &&
+ test_expect_code 1 git cherry-pick base..picked &&
git cherry-pick --quit &&
test_path_is_missing .git/sequencer &&
test_must_fail git update-index --refresh &&
@@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
test_expect_success '--abort to cancel multiple cherry-pick' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
git cherry-pick --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev initial HEAD &&
@@ -131,7 +131,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
test_expect_success '--abort to cancel single cherry-pick' '
pristine_detach initial &&
- test_must_fail git cherry-pick picked &&
+ test_expect_code 1 git cherry-pick picked &&
git cherry-pick --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev initial HEAD &&
@@ -141,7 +141,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
test_expect_success 'cherry-pick --abort to cancel multiple revert' '
pristine_detach anotherpick &&
- test_must_fail git revert base..picked &&
+ test_expect_code 1 git revert base..picked &&
git cherry-pick --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev anotherpick HEAD &&
@@ -151,7 +151,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
test_expect_success 'revert --abort works, too' '
pristine_detach anotherpick &&
- test_must_fail git revert base..picked &&
+ test_expect_code 1 git revert base..picked &&
git revert --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev anotherpick HEAD
@@ -159,7 +159,7 @@ test_expect_success 'revert --abort works, too' '
test_expect_success '--abort to cancel single revert' '
pristine_detach anotherpick &&
- test_must_fail git revert picked &&
+ test_expect_code 1 git revert picked &&
git revert --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev anotherpick HEAD &&
@@ -170,7 +170,7 @@ test_expect_success '--abort to cancel single revert' '
test_expect_success '--abort keeps unrelated change, easy case' '
pristine_detach unrelatedpick &&
echo changed >expect &&
- test_must_fail git cherry-pick picked..yetanotherpick &&
+ test_expect_code 1 git cherry-pick picked..yetanotherpick &&
echo changed >unrelated &&
git cherry-pick --abort &&
test_cmp expect unrelated
@@ -179,7 +179,7 @@ test_expect_success '--abort keeps unrelated change, easy case' '
test_expect_success '--abort refuses to clobber unrelated change, harder case' '
pristine_detach initial &&
echo changed >expect &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo changed >unrelated &&
test_must_fail git cherry-pick --abort &&
test_cmp expect unrelated &&
@@ -194,7 +194,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..picked &&
+ test_expect_code 1 git cherry-pick base..picked &&
test_path_is_missing .git/sequencer &&
echo "resolved" >foo &&
git add foo &&
@@ -218,7 +218,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
test_expect_failure '--abort after last commit in sequence' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..picked &&
+ test_expect_code 1 git cherry-pick base..picked &&
git cherry-pick --abort &&
test_path_is_missing .git/sequencer &&
test_cmp_rev initial HEAD &&
@@ -228,27 +228,27 @@ test_expect_failure '--abort after last commit in sequence' '
test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
test-chmtime -v +0 .git/sequencer >expect &&
- test_must_fail git cherry-pick unrelatedpick &&
+ test_expect_code 128 git cherry-pick unrelatedpick &&
test-chmtime -v +0 .git/sequencer >actual &&
test_cmp expect actual
'
test_expect_success '--continue complains when no cherry-pick is in progress' '
pristine_detach initial &&
- test_must_fail git cherry-pick --continue
+ test_expect_code 128 git cherry-pick --continue
'
test_expect_success '--continue complains when there are unresolved conflicts' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
- test_must_fail git cherry-pick --continue
+ test_expect_code 1 git cherry-pick base..anotherpick &&
+ test_expect_code 128 git cherry-pick --continue
'
test_expect_success '--continue continues after conflicts are resolved' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "c" >foo &&
git add foo &&
git commit &&
@@ -275,7 +275,7 @@ test_expect_success '--continue continues after conflicts are resolved' '
test_expect_success '--continue respects opts' '
pristine_detach initial &&
- test_must_fail git cherry-pick -x base..anotherpick &&
+ test_expect_code 1 git cherry-pick -x base..anotherpick &&
echo "c" >foo &&
git add foo &&
git commit &&
@@ -293,7 +293,7 @@ test_expect_success '--continue respects opts' '
test_expect_success '--signoff is not automatically propagated to resolved conflict' '
pristine_detach initial &&
- test_must_fail git cherry-pick --signoff base..anotherpick &&
+ test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
echo "c" >foo &&
git add foo &&
git commit &&
@@ -311,40 +311,40 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
test_expect_success 'malformed instruction sheet 1' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "resolved" >foo &&
git add foo &&
git commit &&
sed "s/pick /pick/" .git/sequencer/todo >new_sheet &&
cp new_sheet .git/sequencer/todo &&
- test_must_fail git cherry-pick --continue
+ test_expect_code 128 git cherry-pick --continue
'
test_expect_success 'malformed instruction sheet 2' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "resolved" >foo &&
git add foo &&
git commit &&
sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
cp new_sheet .git/sequencer/todo &&
- test_must_fail git cherry-pick --continue
+ test_expect_code 128 git cherry-pick --continue
'
test_expect_success 'malformed instruction sheet 3' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "resolved" >foo &&
git add foo &&
git commit &&
sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
cp new_sheet .git/sequencer/todo &&
- test_must_fail git cherry-pick --continue
+ test_expect_code 128 git cherry-pick --continue
'
test_expect_success 'instruction sheet, fat-fingers version' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "c" >foo &&
git add foo &&
git commit &&
@@ -355,7 +355,7 @@ test_expect_success 'instruction sheet, fat-fingers version' '
test_expect_success 'commit descriptions in insn sheet are optional' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
echo "c" >foo &&
git add foo &&
git commit &&
--
1.7.4.1
^ permalink raw reply related
* [PATCH 06/10] t3502, t3510: clarify cherry-pick -m failure
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
The "cherry-pick persists opts correctly" test in t3510
(cherry-pick-sequence) can cause some confusion, because the command
actually has two points of failure:
1. "-m 1" is specified on the command-line despite the base commit
"initial" not being a merge-commit.
2. The revision range indicates that there will be a conflict that
needs to be resolved.
Although the former error is trapped, and cherry-pick die()s with the
exit status 128, the reader may be distracted by the latter. Fix this
by changing the revision range to something that wouldn't cause a
conflict. Additionally, explicitly check the exit code in
"cherry-pick a non-merge with -m should fail" in t3502
(cherry-pick-merge) to reassure the reader that this failure has
nothing to do with the sequencer itself.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3502-cherry-pick-merge.sh | 2 +-
t/t3510-cherry-pick-sequence.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 0ab52da..e37547f 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -35,7 +35,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '
git reset --hard &&
git checkout a^0 &&
- test_must_fail git cherry-pick -m 1 b &&
+ test_expect_code 128 git cherry-pick -m 1 b &&
git diff --exit-code a --
'
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b6e822e..163ce7d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
test_expect_success 'cherry-pick persists opts correctly' '
pristine_detach initial &&
- test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+ test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
test_path_is_file .git/sequencer/todo &&
--
1.7.4.1
^ permalink raw reply related
* [PATCH 07/10] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
The next patch allows mixing "pick" and "revert" instruction in the
same instruction sheet. By removing the "malformed instruction sheet
2" test in advance, it'll be easier to see the changes made by the
future patches that touch this file.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 15 ++-------------
1 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 163ce7d..1bfbe41 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -309,7 +309,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
grep "Signed-off-by:" anotherpick_msg
'
-test_expect_success 'malformed instruction sheet 1' '
+test_expect_success 'malformed instruction sheet, action' '
pristine_detach initial &&
test_expect_code 1 git cherry-pick base..anotherpick &&
echo "resolved" >foo &&
@@ -320,18 +320,7 @@ test_expect_success 'malformed instruction sheet 1' '
test_expect_code 128 git cherry-pick --continue
'
-test_expect_success 'malformed instruction sheet 2' '
- pristine_detach initial &&
- test_expect_code 1 git cherry-pick base..anotherpick &&
- echo "resolved" >foo &&
- git add foo &&
- git commit &&
- sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
- cp new_sheet .git/sequencer/todo &&
- test_expect_code 128 git cherry-pick --continue
-'
-
-test_expect_success 'malformed instruction sheet 3' '
+test_expect_success 'malformed instruction sheet, object name' '
pristine_detach initial &&
test_expect_code 1 git cherry-pick base..anotherpick &&
echo "resolved" >foo &&
--
1.7.4.1
^ permalink raw reply related
* [PATCH 08/10] revert: move replay_action, replay_subcommand to header
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
In our vision to build a generalized sequencer, the revert builtin
will be left only with argument parsing work before calling into the
sequencer. Since the enums replay_action and replay_subcommand have
nothing to do with this argument parsing, move them to sequencer.h.
However, REVERT and CHERRY_PICK are unsuitable variable names for
publicly exposing, as their purpose is unclear in the global context:
rename them to REPLAY_REVERT and REPLAY_PICK respectively.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 42 +++++++++++++++++-------------------------
sequencer.h | 12 ++++++++++++
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 0a4b215..ed062ea 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,14 +39,6 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand {
- REPLAY_NONE,
- REPLAY_REMOVE_STATE,
- REPLAY_CONTINUE,
- REPLAY_ROLLBACK
-};
-
struct replay_opts {
enum replay_action action;
enum replay_subcommand subcommand;
@@ -73,14 +65,14 @@ struct replay_opts {
static const char *action_name(const struct replay_opts *opts)
{
- return opts->action == REVERT ? "revert" : "cherry-pick";
+ return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
}
static char *get_encoding(const char *message);
static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
{
- return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+ return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
}
static int option_parse_x(const struct option *opt,
@@ -159,7 +151,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
OPT_END(),
};
- if (opts->action == CHERRY_PICK) {
+ if (opts->action == REPLAY_PICK) {
struct option cp_extra[] = {
OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,7 +355,7 @@ static int error_dirty_index(struct replay_opts *opts)
return error_resolve_conflict(action_name(opts));
/* Different translation strings for cherry-pick and revert */
- if (opts->action == CHERRY_PICK)
+ if (opts->action == REPLAY_PICK)
error(_("Your local changes would be overwritten by cherry-pick."));
else
error(_("Your local changes would be overwritten by revert."));
@@ -542,7 +534,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
defmsg = git_pathdup("MERGE_MSG");
- if (opts->action == REVERT) {
+ if (opts->action == REPLAY_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -583,7 +575,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
@@ -607,13 +599,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* However, if the merge did not even start, then we don't want to
* write it at all.
*/
- if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+ if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
- if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+ if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
write_cherry_pick_head(commit, "REVERT_HEAD");
if (res) {
- error(opts->action == REVERT
+ error(opts->action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +629,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
init_revisions(revs, NULL);
revs->no_walk = 1;
- if (opts->action != REVERT)
+ if (opts->action != REPLAY_REVERT)
revs->reverse = 1;
argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -698,7 +690,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
{
struct commit_list *cur = NULL;
const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REVERT ? "revert" : "pick";
+ const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
const char *subject;
int subject_len;
@@ -719,10 +711,10 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
int saved, status, padding;
if (!prefixcmp(bol, "pick")) {
- action = CHERRY_PICK;
+ action = REPLAY_PICK;
bol += strlen("pick");
} else if (!prefixcmp(bol, "revert")) {
- action = REVERT;
+ action = REPLAY_REVERT;
bol += strlen("revert");
} else
return NULL;
@@ -745,7 +737,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
*/
if (action != opts->action) {
const char *action_str;
- action_str = action == REVERT ? "revert" : "cherry-pick";
+ action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
error(_("Cannot %s during a %s"), action_str, action_name(opts));
return NULL;
}
@@ -1080,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)
if (create_seq_dir() < 0)
return -1;
if (get_sha1("HEAD", sha1)) {
- if (opts->action == REVERT)
+ if (opts->action == REPLAY_REVERT)
return error(_("Can't revert as initial commit"));
return error(_("Can't cherry-pick into empty head"));
}
@@ -1097,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
memset(&opts, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
- opts.action = REVERT;
+ opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
@@ -1112,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
int res;
memset(&opts, 0, sizeof(opts));
- opts.action = CHERRY_PICK;
+ opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index f435fdb..9b1b94d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,18 @@
#define SEQ_TODO_FILE "sequencer/todo"
#define SEQ_OPTS_FILE "sequencer/opts"
+enum replay_action {
+ REPLAY_REVERT,
+ REPLAY_PICK
+};
+
+enum replay_subcommand {
+ REPLAY_NONE,
+ REPLAY_REMOVE_STATE,
+ REPLAY_CONTINUE,
+ REPLAY_ROLLBACK
+};
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
--
1.7.4.1
^ permalink raw reply related
* [PATCH 09/10] revert: allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action. Now you can do:
pick fdc0b12 picked
revert 965fed4 anotherpick
For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.
Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 107 ++++++++++++++++++---------------------
sequencer.h | 6 ++
t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++++++
3 files changed, 114 insertions(+), 57 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index ed062ea..50af439 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -459,7 +459,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
return run_command_v_opt(args, RUN_GIT_CMD);
}
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+ struct replay_opts *opts)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -534,7 +535,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
defmsg = git_pathdup("MERGE_MSG");
- if (opts->action == REPLAY_REVERT) {
+ if (action == REPLAY_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -575,7 +576,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
@@ -605,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
write_cherry_pick_head(commit, "REVERT_HEAD");
if (res) {
- error(opts->action == REPLAY_REVERT
+ error(action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -675,54 +676,54 @@ static void read_and_refresh_cache(struct replay_opts *opts)
* assert(commit_list_count(list) == 2);
* return list;
*/
-static struct commit_list **commit_list_append(struct commit *commit,
- struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+ struct commit *operand,
+ struct replay_insn_list **next)
{
- struct commit_list *new = xmalloc(sizeof(struct commit_list));
- new->item = commit;
+ struct replay_insn_list *new = xmalloc(sizeof(*new));
+ new->action = action;
+ new->operand = operand;
*next = new;
new->next = NULL;
return &new->next;
}
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
- struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
{
- struct commit_list *cur = NULL;
- const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
- const char *subject;
- int subject_len;
+ struct replay_insn_list *cur;
for (cur = todo_list; cur; cur = cur->next) {
- sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- subject_len = find_commit_subject(cur->item->buffer, &subject);
+ const char *sha1_abbrev, *action_str, *subject;
+ int subject_len;
+
+ action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ subject_len = find_commit_subject(cur->operand->buffer, &subject);
strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
subject_len, subject);
}
return 0;
}
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
{
unsigned char commit_sha1[20];
- enum replay_action action;
char *end_of_object_name;
int saved, status, padding;
if (!prefixcmp(bol, "pick")) {
- action = REPLAY_PICK;
+ item->action = REPLAY_PICK;
bol += strlen("pick");
} else if (!prefixcmp(bol, "revert")) {
- action = REPLAY_REVERT;
+ item->action = REPLAY_REVERT;
bol += strlen("revert");
} else
- return NULL;
+ return -1;
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
- return NULL;
+ return -1;
bol += padding;
end_of_object_name = bol + strcspn(bol, " \t\n");
@@ -731,37 +732,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- const char *action_str;
- action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
- error(_("Cannot %s during a %s"), action_str, action_name(opts));
- return NULL;
- }
-
if (status < 0)
- return NULL;
+ return -1;
- return lookup_commit_reference(commit_sha1);
+ item->operand = lookup_commit_reference(commit_sha1);
+ if (!item->operand)
+ return -1;
+
+ item->next = NULL;
+ return 0;
}
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
- struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
{
- struct commit_list **next = todo_list;
- struct commit *commit;
+ struct replay_insn_list **next = todo_list;
+ struct replay_insn_list item = { 0, NULL, NULL };
char *p = buf;
int i;
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- commit = parse_insn_line(p, eol, opts);
- if (!commit)
+ if (parse_insn_line(p, eol, &item) < 0)
return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
+ next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
@@ -769,8 +762,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
return 0;
}
-static void read_populate_todo(struct commit_list **todo_list,
- struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
struct strbuf buf = STRBUF_INIT;
@@ -786,7 +778,7 @@ static void read_populate_todo(struct commit_list **todo_list,
}
close(fd);
- res = parse_insn_buffer(buf.buf, todo_list, opts);
+ res = parse_insn_buffer(buf.buf, todo_list);
strbuf_release(&buf);
if (res)
die(_("Unusable instruction sheet: %s"), todo_file);
@@ -835,18 +827,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
die(_("Malformed options sheet: %s"), opts_file);
}
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
- struct commit_list **next;
+ struct replay_insn_list **next;
prepare_revs(&revs, opts);
next = todo_list;
while ((commit = get_revision(&revs)))
- next = commit_list_append(commit, next);
+ next = replay_insn_list_append(opts->action, commit, next);
}
static int create_seq_dir(void)
@@ -944,7 +936,7 @@ fail:
return -1;
}
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
static struct lock_file todo_lock;
@@ -952,7 +944,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
int fd;
fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list, opts) < 0)
+ if (format_todo(&buf, todo_list) < 0)
die(_("Could not format %s."), todo_file);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
@@ -996,9 +988,10 @@ static void save_opts(struct replay_opts *opts)
}
}
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+ struct replay_opts *opts)
{
- struct commit_list *cur;
+ struct replay_insn_list *cur;
int res;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1008,8 +1001,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
read_and_refresh_cache(opts);
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur, opts);
- res = do_pick_commit(cur->item, opts);
+ save_todo(cur);
+ res = do_pick_commit(cur->operand, cur->action, opts);
if (res) {
if (!cur->next)
/*
@@ -1034,7 +1027,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
static int pick_revisions(struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
+ struct replay_insn_list *todo_list = NULL;
unsigned char sha1[20];
read_and_refresh_cache(opts);
@@ -1054,7 +1047,7 @@ static int pick_revisions(struct replay_opts *opts)
if (!file_exists(git_path(SEQ_TODO_FILE)))
return error(_("No %s in progress"), action_name(opts));
read_populate_opts(&opts);
- read_populate_todo(&todo_list, opts);
+ read_populate_todo(&todo_list);
/* Verify that the conflict has been resolved */
if (!index_differs_from("HEAD", 0))
diff --git a/sequencer.h b/sequencer.h
index 9b1b94d..648f7bf 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -19,6 +19,12 @@ enum replay_subcommand {
REPLAY_ROLLBACK
};
+struct replay_insn_list {
+ enum replay_action action;
+ struct commit *operand;
+ struct replay_insn_list *next;
+};
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 1bfbe41..9b19917 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
test_line_count = 4 commits
'
+test_expect_success 'revert --continue continues after cherry-pick' '
+ pristine_detach initial &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ git revert --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'mixed pick and revert instructions' '
+ pristine_detach initial &&
+ test_expect_code 1 git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ oldsha=`git rev-parse --short HEAD~1` &&
+ echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
1.7.4.1
^ permalink raw reply related
* [PATCH 10/10] revert: report fine-grained error messages from insn parser
From: Ramkumar Ramachandra @ 2011-12-14 16:54 UTC (permalink / raw)
To: Git List; +Cc: Jonathan Nieder, Junio C Hamano
In-Reply-To: <1323881677-11117-1-git-send-email-artagnon@gmail.com>
Three kinds of errors can arise from parsing '.git/sequencer/todo':
1. Unrecognized action
2. Missing space after valid action prefix
3. Malformed object name
4. Object name does not refer to a valid commit
Since we would like to make the instruction sheet user-editable in the
future (much like the 'rebase -i' sheet), report more fine-grained
parse errors prefixed with the filename and line number.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 37 ++++++++++++++++++++++++++-----------
1 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 50af439..d106c3c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -705,11 +705,14 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
return 0;
}
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
unsigned char commit_sha1[20];
char *end_of_object_name;
int saved, status, padding;
+ size_t error_len;
if (!prefixcmp(bol, "pick")) {
item->action = REPLAY_PICK;
@@ -717,13 +720,19 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
} else if (!prefixcmp(bol, "revert")) {
item->action = REPLAY_REVERT;
bol += strlen("revert");
- } else
- return -1;
+ } else {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Unrecognized action: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
- if (!padding)
- return -1;
+ if (!padding) {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Missing space after action: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
bol += padding;
end_of_object_name = bol + strcspn(bol, " \t\n");
@@ -732,12 +741,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- if (status < 0)
- return -1;
+ if (status < 0) {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Malformed object name: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
item->operand = lookup_commit_reference(commit_sha1);
- if (!item->operand)
- return -1;
+ if (!item->operand) {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Not a valid commit: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
item->next = NULL;
return 0;
@@ -752,8 +767,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- if (parse_insn_line(p, eol, &item) < 0)
- return error(_("Could not parse line %d."), i);
+ if (parse_insn_line(p, eol, &item, i) < 0)
+ return -1;
next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
--
1.7.4.1
^ permalink raw reply related
* Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))
From: Junio C Hamano @ 2011-12-14 17:42 UTC (permalink / raw)
To: Thomas Rast
Cc: Jonathan Nieder, git, Jeff King,
Nguyễn Thái Ngọc Duy, Michael Haggerty
In-Reply-To: <201112141717.15021.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Jonathan Nieder wrote:
>> Junio C Hamano wrote:
>> > Will merge to 'next' after taking another look.
>>
>> The middle commit looks good. The bottom commit could be improved as
>> discussed at [1], but I guess that can happen in-tree.
>>
>> However, the top commit ("test test-terminal's sanity") still does not
>> seem right to me.
>
> I wasn't under the impression that we were done with this, either :-)
>
>> It makes the same test run three times. Probably I should send an
>> alternate patch to get that sanity-check to run once, but I am also
>> not convinced the sanity-check is needed at all --- wouldn't any test
>> that is relying on output from test_terminal act as a sanity check for
>> it already?
>
> It didn't. Or more precisely, Michael Haggerty ran into the behavior
> of
>
> git rev-parse ... | while read sha; do git checkout $sha; make test; done
>
> couldn't make any sense of it, and reported it on IRC. So in some
> sense, it took infrequent circumstances and two developers' time; next
> time around I'd prefer it to be detected automatically.
>
>> As an aside, I also still believe that running "git shortlog" without
>> explicitly passing "HEAD" when testing how it reacts to [core] pager
>> configuration was a bug and a distraction, hence the patch at [2].
>
> Why not. Some other test should verify how shortlog reacts to the
> tty-ness of stdin, but that's yet another direction.
>
>> I also find Jeff's patch [3] appealing.
>
> Me too, though wonder whether feeding a file full of garbage wouldn't
> be better, so as to trip up commands that try to read only from a
> non-tty stdin.
Well, I guess I was too quick to pull the trigger after sending the
"What's cooking" out. Sorry about that.
On the other hand, I think these require relatively low impact changes
that can be handled in-tree and the downsides of the series like running
prerequisite tests more than once are not serious show stoppers, so it
isn't a disaster ;-)
Thanks both for noticing and commenting. Very much appreciated.
^ permalink raw reply
* Re: Git difftool / mergetool on directory tree
From: Junio C Hamano @ 2011-12-14 17:50 UTC (permalink / raw)
To: Thomas Rast; +Cc: Daniele Segato, Git Mailing List
In-Reply-To: <201112141725.14729.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Daniele Segato wrote:
>> Hi,
>>
>> many diff / merge tool around have the ability to compare a directory
>> tree (meld is one, but there are many).
>>
>> Is there a way to start a difftool or a mergetool on a folder instead of
>> the single file?
>>
>> It would be an handsome feature to git.
>>
>> I googled a little before popping out this question and I only found
>> suggestion on how to open "many" file at once instead of opening them
>> serially but that's not the same thing not as powerful as directory
>> comparison.
>
> Maybe this helps:
>
> http://thread.gmane.org/gmane.comp.version-control.git/144839
>
> It was never applied however.
A later reincarnation of a similar idea was in
http://thread.gmane.org/gmane.comp.version-control.git/184458/focus=184462
but the interest petered out, it seems.
^ permalink raw reply
* Re: [bug?] checkout -m doesn't work without a base version
From: Junio C Hamano @ 2011-12-14 17:54 UTC (permalink / raw)
To: Michael Schubert; +Cc: Pete Harlan, git
In-Reply-To: <4EE8782A.9040507@elegosoft.com>
Michael Schubert <mschub@elegosoft.com> writes:
>> ...
>> + memset(threeway, 0, sizeof(threeway));
>> + while (pos < active_nr) {
>> + int stage;
>> + stage = ce_stage(ce);
>> + if (!stage || strcmp(path, ce->name))
>> + break;
>> + hashcpy(threeway[stage - 1], ce->sha1);
>> + if (stage == 2)
>> + mode = create_ce_mode(ce->ce_mode);
>> + pos++;
>> + ce = active_cache[pos];
>> + }
>> + if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2]))
>> + return error(_("path '%s' does not have necessary versions"), path);
>> ...
>> + ce = make_cache_entry(mode, sha1, path, 2, 0);
>> if (!ce)
>> die(_("make_cache_entry failed for path '%s'"), path);
>> status = checkout_entry(ce, state, NULL);
>
> gcc 4.6.2:
>
> builtin/checkout.c: In function ‘cmd_checkout’:
> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized in this function [-Wuninitialized]
> builtin/checkout.c:160:11: note: ‘mode’ was declared here
Isn't this just your gcc being overly cautious (aka "silly")?
The variable "mode" is assigned to when we see an stage #2 entry in the
loop, and we should have updated threeway[1] immediately before doing so.
If threeway[1] is not updated, we would have already returned before using
the variable in make_cache_entry().
^ permalink raw reply
* Re: Revisiting metadata storage
From: Richard Hartmann @ 2011-12-14 17:59 UTC (permalink / raw)
To: Ronan Keryell; +Cc: Git List
In-Reply-To: <87sjkx8gll.fsf@an-dro.info.enstb.org>
On Tue, Dec 6, 2011 at 23:45, Ronan Keryell
<Ronan.Keryell@hpc-project.com> wrote:
> At least I'm interested and began to dig into it but I do not have a lot
> of time to work on it...
If we can agree on Perl, I can try to help. I don't think I speak
enough Python to be of use with that.
Other people who have an interest in this: Please pipe up so we can
hammer out a rough consensus & roadmap.
Richard
^ permalink raw reply
* Re: [PATCH 1/3] Make commit_tree() take message length in addition to the commit message
From: Junio C Hamano @ 2011-12-14 18:12 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Miles Bader
In-Reply-To: <1323871699-8839-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Justification?
As all 3 primary users of this API feed strbuf.buf to the function, it
would make more sense to change the first parameter to a pointer to a
strbuf, no?
^ permalink raw reply
* Re: [PATCH 2/3] merge: abort if fails to commit
From: Junio C Hamano @ 2011-12-14 18:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Miles Bader
In-Reply-To: <1323871699-8839-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/merge.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index df4548a..e57eefa 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -913,7 +913,9 @@ static int merge_trivial(struct commit *head)
> parent->next->item = remoteheads->item;
> parent->next->next = NULL;
> prepare_to_commit();
> - commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);
> + if (commit_tree(merge_msg.buf, merge_msg.len,
> + result_tree, parent, result_commit, NULL))
> + die(_("failed to write commit object"));
> finish(head, result_commit, "In-index merge");
> drop_save();
> return 0;
Should we die immediately, or should we do some clean-ups after ourselves
before doing so?
In any case, this is a good change that shouldn't be taken hostage to the
unrelated change in patch [1/3].
Thanks.
> @@ -944,7 +946,9 @@ static int finish_automerge(struct commit *head,
> strbuf_addch(&merge_msg, '\n');
> prepare_to_commit();
> free_commit_list(remoteheads);
> - commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);
> + if (commit_tree(merge_msg.buf, merge_msg.len,
> + result_tree, parents, result_commit, NULL))
> + die(_("failed to write commit object"));
> strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
> finish(head, result_commit, buf.buf);
> strbuf_release(&buf);
^ permalink raw reply
* Re: [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls
From: Jeff King @ 2011-12-14 18:16 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, schwab
In-Reply-To: <1323871631-2872-5-git-send-email-kusmabite@gmail.com>
On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote:
> This avoids us from accidentally dropping state, possibly leading
> to unexpected behaviour.
I do think this is fine in a "be extra cautious" kind of way.
> This is especially important on Windows, where the maximum size of
> the environment is 32 kB.
But does your patch actually detect that? As Andreas pointed out, these
limits don't typically come into play at setenv time. Instead, the
environment is allocated on the heap, and then the result is passed to
exec/spawn, which will fail.
So your patch is really detecting a failure to malloc, not an overflow
of the environment size, and Windows is just as (un)likely to run out of
heap as any other platform.
You can check how your platform behaves by applying this patch:
diff --git a/git.c b/git.c
index f10e434..57f6b12 100644
--- a/git.c
+++ b/git.c
@@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv)
alias_argv[i] = (*argv)[i];
alias_argv[argc] = NULL;
+ /* make gigantic environment */
+ {
+ int len = 256 * 1024;
+ char *buf = xmalloc(len);
+ memset(buf, 'z', len);
+ buf[len-1] = '\0';
+ if (setenv("FOO", buf, 1))
+ die("setenv failed");
+ }
+
ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
if (ret >= 0) /* normal exit */
exit(ret);
and then running:
git -c alias.foo='!echo ok' foo
which yields:
fatal: cannot exec 'echo ok': Argument list too long
on Linux.
-Peff
PS I tried to come up with an invocation of git that would demonstrate
this, but it turns out it's really hard. The contents of environment
variables we set are either constants, come from the environment (so
they can't be too big already!), or come from filesystem paths. So
it's possible to overflow now, but you have to have a nearly-full
environment in the first place, and then have a long path that tips
it over the limit.
^ permalink raw reply related
* Re: [PATCH 3/3] Do not create commits whose message contains NUL
From: Junio C Hamano @ 2011-12-14 18:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Miles Bader
In-Reply-To: <1323871699-8839-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> We assume that the commit log messages are uninterpreted sequences of
> non-NUL bytes (see Documentation/i18n.txt). However the assumption
> does not really stand out and it's quite easy to set an editor to save
> in a NUL-included encoding. Currently we silently cut at the first NUL
> we see.
>
> Make it more obvious that NUL is not welcome by refusing to create
> such commits. Those who deliberately want to create them can still do
> with hash-object.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Limiting the Porcelain layer to deal only with reasonable text encodings
(yes, I am declaring that utf16 is not among them) is perfectly fine, but
I was somehow hoping that you would allow the option for the low-level
function commit_tree() to create a commit object with binary blob in the
body part, especially after seeing the patch 1/3 to do so.
Certainly that kind of usage would not give the binary blob literally in
"git log" output, but it is with or without the issue around NUL byte. A
custom program linked with commit.c to call commit_tree() may not be using
the data structure to store anything that is meant to be read by "git log"
to begin with.
Not a strong veto at all, just throwing out something to think about.
^ permalink raw reply
* Re: [PATCH 3/3] Do not create commits whose message contains NUL
From: Jeff King @ 2011-12-14 18:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git, Miles Bader
In-Reply-To: <7vzkevow2j.fsf@alter.siamese.dyndns.org>
On Wed, Dec 14, 2011 at 10:19:16AM -0800, Junio C Hamano wrote:
> Limiting the Porcelain layer to deal only with reasonable text encodings
> (yes, I am declaring that utf16 is not among them) is perfectly fine, but
> I was somehow hoping that you would allow the option for the low-level
> function commit_tree() to create a commit object with binary blob in the
> body part, especially after seeing the patch 1/3 to do so.
>
> Certainly that kind of usage would not give the binary blob literally in
> "git log" output, but it is with or without the issue around NUL byte. A
> custom program linked with commit.c to call commit_tree() may not be using
> the data structure to store anything that is meant to be read by "git log"
> to begin with.
I'm happy to ignore custom programs linking against internal git code,
but what should "git commit-tree" do?
My gut feeling is that it should store the literal binary contents.
However, I don't think this has ever been the case. Even in the initial
version of commit-tree.c, we read the input line-by-line and sprintf it
into place.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox