* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehwdcob3.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 03:34:08PM -0800, Junio C Hamano wrote:
> > We _could_ modify credential_match() to automatically reject such a
> > pattern at that level,...
>
> I do not think that is such a good idea to modify "match()" function
> either, as I agree match with empty has its uses, but that does not stop
> "rewrite_credential_file()" from being safe by default, no? After all, the
> one that makes the decision to drop things that match the pattern is that
> function (it chooses to give NULL to match_cb).
Yeah, you could move it down to that level, but there isn't much point.
rewrite_credential_file is unique to credential-store, and the only two
callers are store_credential (which has its own, stricter rules already)
and remove_credential, which we are modifying here.
Note that I didn't bother with the same safety valve for
credential-cache. It is, after all, a cache that will go away eventually
anyway, so safety is less interesting.
Third-party helpers will have to do their own checks anyway, as in
general I don't plan on them linking directly against git code.
Speaking of which, I hackishly ported Jay's osxkeychain helper to the
new format last night. I'll try to clean that up and post it tonight.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111209231800.GA14376@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I think that's a reasonable compromise. Instead of the patch I
> posted earlier, how about this:
>
> diff --git a/credential-store.c b/credential-store.c
> index a2c2cd0..26f7589 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
>
> static void remove_credential(const char *fn, struct credential *c)
> {
> - rewrite_credential_file(fn, c, NULL);
> + /*
> + * Sanity check that we actually have something to match
> + * against. The input we get is a restrictive pattern,
> + * so technically a blank credential means "erase everything".
> + * But it is too easy to accidentally send this, since it is equivalent
> + * to empty input. So explicitly disallow it, and require that the
> + * pattern have some actual content to match.
> + */
> + if (c->protocol || c->host || c->path || c->username)
> + rewrite_credential_file(fn, c, NULL);
> }
Looks very sane.
> We _could_ modify credential_match() to automatically reject such a
> pattern at that level,...
I do not think that is such a good idea to modify "match()" function
either, as I agree match with empty has its uses, but that does not stop
"rewrite_credential_file()" from being safe by default, no? After all, the
one that makes the decision to drop things that match the pattern is that
function (it chooses to give NULL to match_cb).
> So the "empty pattern" does actually have a use, from the end-users's
> point of view. It's just that with removal, it's a little more dangerous
> and a little less likely to be useful (as compared to lookup).
>
> -Peff
^ permalink raw reply
* Re: [PATCHv2 12/13] credentials: add "store" helper
From: Jeff King @ 2011-12-09 23:19 UTC (permalink / raw)
To: git
In-Reply-To: <20111206062305.GL29233@sigill.intra.peff.net>
On Tue, Dec 06, 2011 at 01:23:05AM -0500, Jeff King wrote:
> +int main(int argc, const char **argv)
> +{
> + const char * const usage[] = {
> + "git credential-store [options] <action>",
> + NULL
> + };
> + const char *op;
> + struct credential c = CREDENTIAL_INIT;
> + char *file = NULL;
> + struct option options[] = {
> + OPT_STRING_LIST(0, "file", &file, "path",
> + "fetch and store credentials in <path>"),
> + OPT_END()
> + };
Eek, this should be OPT_STRING, of course. I'll fix it in my next
re-roll, but I wanted to mention it in case anybody is reviewing v2.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkf1fwvn.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It works, and it detects truncated output both ways properly (I know
> > because I had to update every test, since the old output was missing the
> > end-of-credential marker).
> >
> > It makes me a little sad, because the original format (relying on EOF)
> > was so Unix-y.
>
> It saddens me, too. A reasonable middle ground would be to stop treating
> an empty input as "no restriction" but "never matches".
>
> I suspect that it is far more likely for a helper to fail (due to
> configuration errors, for example) before it produces any output than
> after it gives some but not all output lines.
Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:
diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
static void remove_credential(const char *fn, struct credential *c)
{
- rewrite_credential_file(fn, c, NULL);
+ /*
+ * Sanity check that we actually have something to match
+ * against. The input we get is a restrictive pattern,
+ * so technically a blank credential means "erase everything".
+ * But it is too easy to accidentally send this, since it is equivalent
+ * to empty input. So explicitly disallow it, and require that the
+ * pattern have some actual content to match.
+ */
+ if (c->protocol || c->host || c->path || c->username)
+ rewrite_credential_file(fn, c, NULL);
}
static int lookup_credential(const char *fn, struct credential *c)
We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:
[credential "https://example.com/foo.git"]
helper = ...
[credential "https://example.com"]
helper = ...
[credential "https://"]
helper = ...
[credential]
helper = ...
The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).
So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).
-Peff
^ permalink raw reply related
* Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2
From: Ramkumar Ramachandra @ 2011-12-09 23:03 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1323445326-24637-1-git-send-email-artagnon@gmail.com>
Hi,
I'm off on a short vacation next week. I've already fixed up the
series in response to most (if not all) reviews. If anyone's
interested in putting in some finishing touches, it's available here:
https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 195e68e
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 22:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111209195042.GG20913@elie.hsd1.il.comcast.net>
Hey,
Jonathan Nieder wrote:
> You can make git misbehave before applying the commit.
Right, it's all in the discussion surrounding $gmane/184031. I
couldn't recall because it was so long ago :\
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-09 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vaa71hd5l.fsf@alter.siamese.dyndns.org>
On Fri, 09 Dec 2011 18:23:50 +0100, Junio C Hamano <gitster@pobox.com>
wrote:
> "Frans Klaver" <fransklaver@gmail.com> writes:
>
>>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want
>>> without
>>> this much trouble?
>>
>> I just had a good look through the man page of access(2), and I think
>> it depends. access works for the real uid, which is what I attempted
>> to implement in the above check as well. However, do we actually need
>> to use the real uid or do we need the set uid (geteuid(2))?
>
> Does it matter? We do not use seteuid or setegid ourselves and we do not
> expect to be installed as owned by root with u+s bit set.
That's what I thought, but needed to know for sure that this was the case.
> access(2) checks with real uid exactly because it would not make a
> difference to normal user level programs _and_ it makes it easier for a
> suid programs to check with the real identity, and our use case falls
> into the former, no?
>
Certainly looks like. Thanks. I'll reroll somewhere next week.
Frans
^ permalink raw reply
* Re: [PATCH 1/2] index_pack: indent find_unresolved_detals one level
From: Junio C Hamano @ 2011-12-09 21:27 UTC (permalink / raw)
To: pclouds; +Cc: git, Shawn Pearce
In-Reply-To: <4ee0be67.05c1e70a.1956.ffff800b@mx.google.com>
pclouds@gmail.com writes:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> The next patch puts most of the code in one level deeper. By indenting
> separately, it'd be easier to see the actual changes.
Yuck.
Isn't it a sign that "the next patch" should perhaps be helped by a small
helper function that does whatever the part you are indenting here?
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-09 21:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0m2veE8FmFVTPEqNAmbtvm1sWVHtFt0QOWU=huQFafeBw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> 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.
That might be a nice thing to do anyway, but I don't see how it would
solve anything. The new "git commit" API would presumably return an
integer or enum value to indicate the result of trying to commit.
Tests in the testsuite for the "git commit" API would use the "git
commit" command, which would expose the newly fine-grained values
somehow. And other people scripting but wanting the porcelain to take
care of basic UI would benefit, too. Right?
Actually, I think cherry-pick returning a positive exit status for
"nothing left to commit after resolving conflicts" would be sensible.
It is "I did what you asked but need your help to determine the final
content of the commit or decide to skip it", rather than "you asked
for something unsensible and I am bailing out".
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Jonathan Nieder @ 2011-12-09 21:00 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=a=-4N4aZHuu=5zkNtwmfLsog5WkBVbuJAbYpvaLUsAg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> - 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.
Ok. I'm fuzzy on the details, but is it possible to make this change
in such a way as to make that obvious? For example, perhaps this
should be split into several tests: one to check that such mistaken
use of "-m 1" with non-merge commits correctly interrupts the
cherry-pick and pleads to the user for advice (should it?), another to
check that doing so produces an exit status of 128 (if it should), and
another to make sure that doing so, fixing things up somehow, and
resuming the sequence allows the effect of "-m 1" to carry over to
later commits.
^ permalink raw reply
* Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-09 20:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209194727.GF20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
> Can cur->item->buffer be NULL?
As Junio correctly points out in $gmane/186365, no. To quote him:
parse_insn_line() uses lookup_commit_reference() which
calls parse_object() on the object name (and if it is a tag, deref_tag()
will parse the tagged object until we see something that is not a tag), so
we know cur->item is parsed before we see it
>> Also, remove the unnecessary check on cur->item: the previous patch
>> makes sure that instruction sheets with missing commit subjects are
>> parsable.
>
> But now I am confused
> [...]
This part of the commit message is simply awful. Replaced with:
Also remove the unnecessary check on cur->item->buffer:
the lookup_commit_reference() call in parse_insn_line() has already
made sure of this.
-- Ram
^ permalink raw reply
* Re: [PATCH 8/9] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2011-12-09 20:47 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-9-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -719,8 +719,10 @@ 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];
I know that this function does not call git_path() again before the
value is used, so this is safe today, but I do not trust people in the
future to preserve that property (for example, maybe someone will want
to call get_sha1() earlier). Why not wait to call git_path() when it
is time to use the value it returns?
^ permalink raw reply
* 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
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