* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Jonathan Nieder @ 2011-12-09 20:37 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0mEP5nDgdosOiquQ_FWbNRZesi38NeCD_yGPvJ8JQxkGg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> I've noticed that
> the diffing algorithm performs especially badly for t/*.sh -- rebasing
> tests is generally a huge pain.
No clue about this particular situation, but I suspect the general
cause for such rebasing trouble is adding tests at the end of the file
(or some other contended place). Better to figure out a logical place
for each test and put it there from the start.
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Ramkumar Ramachandra @ 2011-12-09 20:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209202149.GH20913@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
> [...]
>> --- 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_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 &&
>
> Encountered conflicts, preserving options, but the exit is with status
> 128? Smells like a bug.
No bug. Notice that "-m 1" is used when "initial" isn't a merge
commit. But yeah, I should probably clarify this by changing the
revision range to "initial..anotherpick" so as not to distract the
user.
-- Ram
^ permalink raw reply
* Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2
From: Jonathan Nieder @ 2011-12-09 20:34 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> - "revert: report fine-grained error messages from insn parser" arises
> from Jonathan's request to split "revert: allow mixed pick and
> revert instructions".
Just to be clear: I wasn't directly requesting that you do anything.
If I were, then you could carefully read my requirements, fulfill
them, and you would be done.
Instead, I was reviewing the patch and giving my reaction. After
receiving that information, one has plenty of choices:
- add documentation to avoid the confusion the reaction was based on
- rework to fix the underlying problem that caused the reaction
- think carefully about it, conclude that the reviewer is crazy, and
ignore it
- drop the patch
- send out a call for help to get others to help work on the
underlying problem
- ask for clarification
...
>From my point of view as a reviewer, I am happiest when someone
figures out how I missed the point and comes up with some fix that
addresses the underlying problem instead (and, incidentally, gets rid
of the symptom that provoked my reaction on the way).
Well, you get the idea.
^ permalink raw reply
* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Ramkumar Ramachandra @ 2011-12-09 20:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209202449.GI20913@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> By removing the "malformed instruction sheet
>> 2" test in advance, it'll be easier to see the changes made by the
>> next patch.
>
> So, this is a regression in test coverage without a redeeming upside
> other than allowing the next patch to be prettier. Naturally, I don't
> like it.
Without this patch, the diffs of _all_ the future commits in this
series touching this file are totally unreadable. I've noticed that
the diffing algorithm performs especially badly for t/*.sh -- rebasing
tests is generally a huge pain.
-- Ram
^ permalink raw reply
* Re: [PATCH 7/9] revert: allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-12-09 20:26 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-8-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all instructions use
> the same action. Now you can do:
>
> pick fdc0b12 picked
> revert 965fed4 anotherpick
>
> For cherry-pick and revert, this means that a 'git cherry-pick
> --continue' can continue an ongoing revert operation and viceversa.
Sounds like a good thing.
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -39,7 +39,7 @@ static const char * const cherry_pick_usage[] = {
> NULL
> };
>
> -enum replay_action { REVERT, CHERRY_PICK };
> +enum replay_action { REPLAY_REVERT, REPLAY_PICK };
What does this have to do with it?
^ permalink raw reply
* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Jonathan Nieder @ 2011-12-09 20:24 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-7-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> By removing the "malformed instruction sheet
> 2" test in advance, it'll be easier to see the changes made by the
> next patch.
So, this is a regression in test coverage without a redeeming upside
other than allowing the next patch to be prettier. Naturally, I don't
like it.
^ permalink raw reply
* Re: [PATCH] am: don't persist keepcr flag
From: Junio C Hamano @ 2011-12-09 18:50 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git
In-Reply-To: <1323415845-11826-1-git-send-email-martin.von.zweigbergk@gmail.com>
Makes sense; thanks.
^ permalink raw reply
* Re: [PATCH] git symbolic-ref: documentation fix
From: Junio C Hamano @ 2011-12-09 18:07 UTC (permalink / raw)
To: mhagger; +Cc: git
In-Reply-To: <1323271216-18237-1-git-send-email-mhagger@alum.mit.edu>
Thanks; will queue.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Junio C Hamano @ 2011-12-09 18:24 UTC (permalink / raw)
To: Jakub Narebski
Cc: Ramkumar Ramachandra, Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <m34nx9j2fc.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> writes:
> Well, I think it doesn't have to be true: there can be some options
> like e.g. '-n' / '--dry-run' that are common to all subcommands, and
> in my opinion they could come before subcommand name.
That is just a crazy talk.
If you have three subcommands a, b and c, and only a and b shares -n, c
must learn to reject the option that does not apply. If you have more
subcommands and you need to add an option to one of them, you are forcing
logic to reject that new option to all other subcommands.
That is not a proper way to share option specification. Different
subcommands support different options, and if you want to share some
options among them, you add a new facility to _allow_ them to share _some_
options, while still allowing them to keep their own option specification
table. Otherwise the resulting mess will be unmaintainable.
^ permalink raw reply
* Re: [PATCH, v5] git-tag: introduce --cleanup option
From: Junio C Hamano @ 2011-12-09 18:04 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git, Jeff King
In-Reply-To: <1323226905-31418-1-git-send-email-kirill@shutemov.name>
Looks sane; thanks, will queue.
^ permalink raw reply
* Re: [PATCH] diff/status: print submodule path when looking for changes fails
From: Junio C Hamano @ 2011-12-08 19:15 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Seth Robertson, git
In-Reply-To: <4EDFDF96.9030601@web.de>
Thanks.
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Jonathan Nieder @ 2011-12-09 20:21 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Since cf3e2486 (revert: Propagate errors upwards from do_pick_commit,
> 2011-08-04), 'git cherry-pick' has three different ways of failing:
>
> 1. die() with the exit status 128.
> 3. error() out with exit status -1.
> 2. exit with positive exit status to indicate a conflict.
I think your list-item counter is showing a little jitter. :)
error() does not produce exit status -1, and any situation other than
propagating exit status from a user-defined script in which git exits
with status 255 is a bug (yes, I know there are a couple, though none
I know of in cherry-pick code paths).
Hasn't cherry-pick had two ways to exit with failing status like "git
merge" does (conflicts versus error that didn't even allow us to
start) since the very beginning?
[...]
> So, replace all instances of 'test_must_fail' with
> 'test_expect_code' to check the exit status explicitly.
Sounds like a sensible idea. Probably this one sentence, plus a quick
note on the user-visible exit status convention, would suffice for
explaining it.
> --- 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_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 &&
Encountered conflicts, preserving options, but the exit is with status
128? Smells like a bug.
[...]
> @@ -88,12 +88,12 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress
>
> test_expect_success '--abort requires cherry-pick in progress' '
> pristine_detach initial &&
> - test_must_fail git cherry-pick --abort
> + test_expect_code 128 git cherry-pick --abort
I don't think the exit status is important for this one. (I.e., I can
imagine some future version of cherry-pick using different small
positive integers to refer to different reasons for --abort to fail,
and I don't think that would be a problem or break anything.)
[...]
> @@ -179,9 +179,9 @@ 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_expect_code 128 git cherry-pick --abort &&
Likewise here.
Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet
From: Ramkumar Ramachandra @ 2011-12-09 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <7vty59ec6w.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano wrote:
> Also if you are using strcspn() why use a hand-rolled loop instead of
> strspn()?
Honestly, it didn't occur to me: this is the first time I'm using
either strcspn() or stcspn().
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet
From: Junio C Hamano @ 2011-12-09 20:12 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323445326-24637-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Tolerate extra spaces and tabs as part of the the field separator in
> '.git/sequencer/todo', for people with fat fingers.
>
> Requested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
I did not request it, by the way. I actually was hinting to start tight
and later loosen the parsing.
Also if you are using strcspn() why use a hand-rolled loop instead of
strspn()?
^ permalink raw reply
* Re: [PATCH 1/2] t3401: modernize style
From: Jonathan Nieder @ 2011-12-09 20:07 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Martin von Zweigbergk, git, Junio C Hamano
In-Reply-To: <CALkWK0mip_pzUDQO=YLxaVwsWEAUdrk_EKcNG94Xr5+N+kzBtw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> The motivation is unclear: lazy afternoon? :P
Perhaps he was reading the list and after noticing a few patches in
the same vein, realized that this test script could be made easier to
read, too.
[...]
> Martin von Zweigbergk wrote:
>> + echo First > A &&
>> + git update-index --add A &&
>> + git commit -m "Add A." &&
>
> Style nit: >[^ ] is prevalent FWIW.
At first it wasn't clear to me what you meant here. Was it that
quoted text in an email should start with a non-space character, like
a tab?
Finally I caught on that you mean that redirection operators tend to
be flush against the filename they are redirecting to.
[...]
>> + test ! -d .git/rebase-apply
>> +'
>
> While at it, why not change this "test ! -d" to
> "test_path_is_missing"?
Sounds like a useful hint. The benefits are that it would catch
failures that make .git/rebase-apply into an ordinary file, and more
useful output from "sh t3401-* -v -i" when the test fails. The
main downside I can think of is that the test script would not run
against versions of the test harness before v1.7.3.3~5^2~1 (test-lib:
user-friendly alternatives to test [-d|-f|-e], 2010-08-10).
The patch looks good to me, too. Thanks, both.
Sincerely,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet
From: Ramkumar Ramachandra @ 2011-12-09 19:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209194003.GE20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> +++ b/builtin/revert.c
>> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>> } else
>> return NULL;
>>
>> + /* Eat up extra spaces/ tabs before object name */
>> + while (*bol == ' ' || *bol == '\t')
>> + bol += 1;
>> - end_of_object_name = bol + strcspn(bol, " \n");
>
> Why not use strspn? What happens if I use a tab immediately
> after the pick/revert keyword?
:facepalm: Fixed. Inter-diff:
diff --git a/builtin/revert.c b/builtin/revert.c
index b976562..be0686d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -716,20 +716,22 @@ static struct commit *parse_insn_line(
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;
/* Eat up extra spaces/ tabs before object name */
- while (*bol == ' ' || *bol == '\t')
- bol += 1;
+ padding = strspn(bol, " \t");
+ if (!padding)
+ return NULL;
+ bol += padding;
end_of_object_name = bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
--
Thanks.
-- Ram
^ permalink raw reply related
* Re: [PATCH 2/2] t3401: use test_commit in setup
From: Jonathan Nieder @ 2011-12-09 19:57 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Martin von Zweigbergk, git, Junio C Hamano
In-Reply-To: <CALkWK0k-dL3xZ+dyqdACj7man-Q2QrAPZCCMhXiX0WNGZHv6Fw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Martin von Zweigbergk wrote:
>> Simplify t3401 by using test_commit in the setup. This lets us refer
>> to commits using their tags and there is no longer a need to create
>> the branch my-topic-branch-merge. Also, the branch master-merge points
>> to the same commit as master (even before this change), so that branch
>> does not need to be created either.
>
> The terms "tag" and "branch" here have no significance, so
> de-emphasizing them to "ref" is probably a good idea.
Wha? No, "tag" refers to refs under refs/tags/, and "branch" refers to
refs/heads/, just like usual.
I like the patch, for what it's worth.
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Jonathan Nieder @ 2011-12-09 19:50 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0ki1r5AqYb8qyGHUNupTAhCa2TKwVgkrCpLr+zo_pCy9g@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Not sure what you're talking about. I was saying that any commit that
>> goes from "git can segfault in such-and-such circumstance" to "git no
>> longer segfaults in that circumstance" should loudly proclaim so!
>
> That's what I'm confused about: how can I make git misbehave (let
> alone segfault) after applying this commit?
You can make git misbehave before applying the commit. Applying the
patch fixes it. Unfortunately the patch description does not actually
say so, and at least my small mind is not able to read between the
lines in a hurry to deduce it.
Sorry for the lack of clarity,
Jonathan
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Ramkumar Ramachandra @ 2011-12-09 19:49 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209192919.GB20913@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
>
> So, is it a bad test?
I'll just drop this test.
>> Way starts with creating an API for "git commit".
>
> Not sure what this means.
It certainly doesn't make any sense when you quote it like that. What
I meant is that: by convention, 'git cherry-pick' is supposed to exit
with positive status on conflict. Conversely, this means that a
positive exit status can be interpreted as a conflict, but this is
clearly not the case here. How do we fix this problem? By creating
an API for "git commit", not by shelling out like this and letting it
take over the exit status.
-- Ram
^ permalink raw reply
* Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()
From: Jonathan Nieder @ 2011-12-09 19:47 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> 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.
Nice. Thanks for explaining.
> Also, remove the unnecessary check on cur->item: the previous patch
> makes sure that instruction sheets with missing commit subjects are
> parsable.
I guess you mean the check that cur->item->buffer is non-NULL. But
now I am confused --- what would that have to do with instruction
sheets with missing commit subjects? If cur->item->buffer is NULL,
isn't find_commit_subject going to segfault regardless?
Can cur->item->buffer be NULL?
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 19:44 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111209193527.GD20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
>>> Ramkumar Ramachandra wrote:
>
>>>> [...]
>>>> While at it, also fix a bug: currently, we use a commit-id-shaped
>>>> buffer to store the word after "pick" in '.git/sequencer/todo'. This
>>>> is both wasteful and wrong because it places an artificial limit on
>>>> the line length. Eliminate the need for the buffer altogether, and
>>>> add a test demonstrating this.
>>>
>>> Reading the above does not make it at all obvious that I should want
>>> to apply this patch because otherwise my prankster friend can cause my
>>> copy of git to crash or run arbitrary code by putting a long commit
>>
>> Working backwards:
>> get_sha1() is what will finally misbehave: how?
>
> Not sure what you're talking about. I was saying that any commit that
> goes from "git can segfault in such-and-such circumstance" to "git no
> longer segfaults in that circumstance" should loudly proclaim so!
That's what I'm confused about: how can I make git misbehave (let
alone segfault) after applying this commit? In the previous message,
I was trying to work backwards to figure that out -- I clearly didn't
get anywhere. In other words: what happens if an overly long (a
length that doesn't fit into a size_t) line is present? How does
strchrnul() behave? How do we work around this without imposing some
kind of artificial hard limit?
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet
From: Jonathan Nieder @ 2011-12-09 19:40 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> +++ b/builtin/revert.c
> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
> } else
> return NULL;
>
> + /* Eat up extra spaces/ tabs before object name */
> + while (*bol == ' ' || *bol == '\t')
> + bol += 1;
> - end_of_object_name = bol + strcspn(bol, " \n");
Why not use strspn? What happens if I use a tab immediately
after the pick/revert keyword?
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Jonathan Nieder @ 2011-12-09 19:35 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> [...]
>>> While at it, also fix a bug: currently, we use a commit-id-shaped
>>> buffer to store the word after "pick" in '.git/sequencer/todo'. This
>>> is both wasteful and wrong because it places an artificial limit on
>>> the line length. Eliminate the need for the buffer altogether, and
>>> add a test demonstrating this.
>>
>> Reading the above does not make it at all obvious that I should want
>> to apply this patch because otherwise my prankster friend can cause my
>> copy of git to crash or run arbitrary code by putting a long commit
>
> Working backwards:
> get_sha1() is what will finally misbehave: how?
Not sure what you're talking about. I was saying that any commit that
goes from "git can segfault in such-and-such circumstance" to "git no
longer segfaults in that circumstance" should loudly proclaim so!
Otherwise, when my script is causing git to segfault, I will not know
which commits to point my risk-averse sysadmin to when advocating
upgrading our copy of git.
^ permalink raw reply
* Re: [PATCH 2/9] revert: make commit subjects in insn sheet optional
From: Jonathan Nieder @ 2011-12-09 19:32 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> In addition to literal SHA-1 hexes, expressions like
> the following are valid object names in the instruction sheet:
>
> featurebranch~4
> rr/revert-cherry-pick-continue^2~12@{12 days ago}
Glad to see this new mention. My comment from reviewing the last
iteration doesn't seem to have been addressed, though. Will follow
up there.
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-09 19:29 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0m_6yGuyLt-VqvRZkAiDoSxES8KeNzeXaejYRCpW=HAOg@mail.gmail.com>
Ramkumar Ramachandra wrote:
>> Ramkumar Ramachandra wrote:
>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
>>> +test_expect_success 'commit set passed through --all' '
>>> + pristine_detach initial &&
>>> + test_expect_code 1 git cherry-pick --all &&
>>> + git cherry-pick --continue
>>> +'
[...]
> This one's actually quite interesting. "git cherry-pick --all" first
> tries to apply everything from "intial" to "yetanotherpatch" (both
> inclusive) -- its first "git commit" invocation returns 1, refusing to
> create an empty commit. Then when we say "--continue", it notices
> that the worktree and index are clean, removes "initial" from the
> instruction sheet and executes everything else as usual. This is
> something we should attempt to fix
So, is it a bad test? Was the initial command crazy and ill-defined
enough that no one would actually do that? Is the response to the
command incorrect, meaning that the new test should be instead
checking for some different result with test_expect_failure?
I only mentioned --all in the first place because it is a "revision
pseudo-option" (i.e., option starting with "--" whose position on the
rev-list command line matters) and gets handled by a slightly
different revision parsing code path than foo..bar. There are other
revision pseudo-options that are easier to control and might make for
a better test if it's wanted, like --remotes=foo.
> Way starts with creating an API for "git commit".
Not sure what this means.
^ 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