* 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
* Re: [PATCH] tag deletions not rejected with receive.denyDeletes= true
From: Junio C Hamano @ 2011-12-09 19:15 UTC (permalink / raw)
To: Jerome DE VIVIE; +Cc: git
In-Reply-To: <18683796.591323420674000.JavaMail.root@promailix.prometil.com>
Jerome DE VIVIE <j.devivie@prometil.com> writes:
> Hello,
>
> I have try to deny tag deletion over push using denyDeletes parameter:
>
> git config --system receive.denyDeletes true
> git daemon --reuseaddr --base-path=.. --export-all --verbose --enable=receive-pack
>
> I can push tag deletions despite what the internet says (http://progit.org/book/ch7-1.html#receivedenydeletes). I don't know if it is a bug. Could you have a look, pls ? Thank you
The code seems to be written in such a way that it _explicitly_ wants to
limit the effect of the configuration only to branches. The change was
introduced by a240de1 (Introduce receive.denyDeletes, 2008-11-01) and the
motivation was explained as:
Introduce receive.denyDeletes
Occasionally, it may be useful to prevent branches from getting deleted from
a centralized repository, particularly when no administrative access to the
server is available to undo it via reflog. It also makes
receive.denyNonFastForwards more useful if it is used for access control
since it prevents force-updating by deleting and re-creating a ref.
So I would have to say your "the internet" is wrong.
Our documentation can also use some updates, as it dates to the days back
when we more liberally used "refs" and "branches" interchangeably.
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Ramkumar Ramachandra @ 2011-12-09 19:11 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209190236.GA20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> [...]
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> and Junio can add his sign-off below that when applying some version
> to his tree. Bonus points if you mention what tweaks you made when
> you are not just passing it on.
True, the signoff-chain does look like quite a mess now :P
[rr: minor improvements, tests] should be fine.
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ 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
>> +'
>
> What does this test mean? "git cherry-pick --all" should report a
> spurious conflict, and then --continue will clean it up automagically
> and all will be well?
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 in the future: I think the Right
Way starts with creating an API for "git commit".
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-09 19:02 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-10-git-send-email-artagnon@gmail.com>
Hey,
Ramkumar Ramachandra wrote:
> From: Jonathan Nieder <jrnieder@gmail.com>
[...]
> Future callers as other commands are built in (am, rebase, sequencer)
> may find it easier to pass rev-list options to this machinery in
> already-parsed form. So, teach cmd_cherry_pick and cmd_revert to
> parse the rev-list arguments in advance and pass the commit set to
> pick_revisions() as a "struct rev_info".
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This sign-off chain suggests that the life of the patch happened in
three stages:
- first, you wrote the patch or some component of it and passed it to
me.
- then, I accepted it, tweaked it so much that I felt the need to claim
authorship and save you from blame (hence the "From:" field), and
passed it on to Junio.
- finally, Junio applied it to some tree.
and that you are sending out a copy of the patch Junio applied for
additional comments.
But in fact, this started with a patch from me (I don't rememember
whether it was signed off, but that doesn't matter --- I happily
retroactively sign off on it) and now you are sending it to the list
and Junio for comments. So I would think it should simply say
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
and Junio can add his sign-off below that when applying some version
to his tree. Bonus points if you mention what tweaks you made when
you are not just passing it on.
[...]
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
> test_cmp expect actual
> '
>
> +test_expect_success 'empty commit set' '
> + pristine_detach initial &&
> + test_expect_code 128 git cherry-pick base..base
> +'
> +
> +test_expect_success 'commit set passed through --all' '
> + pristine_detach initial &&
> + test_expect_code 1 git cherry-pick --all &&
> + git cherry-pick --continue
> +'
What does this test mean? "git cherry-pick --all" should report a
spurious conflict, and then --continue will clean it up automagically
and all will be well?
^ 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