* 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
* Re: [PATCH 2/2] t3401: use test_commit in setup
From: Ramkumar Ramachandra @ 2011-12-09 19:02 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Junio C Hamano
In-Reply-To: <1323449952-14161-2-git-send-email-martin.von.zweigbergk@gmail.com>
Hi again,
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. Isn't the truth
more like: instead of creating commits and creating refs to track
those commits by hand, use test_commit to achieve the same result in a
single step?
Cheers.
-- Ram
^ permalink raw reply
* Re: [PATCH 1/2] t3401: modernize style
From: Ramkumar Ramachandra @ 2011-12-09 18:51 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Junio C Hamano
In-Reply-To: <1323449952-14161-1-git-send-email-martin.von.zweigbergk@gmail.com>
Hi Martin,
No cover letter, so I'm assuming these are just two random one-off
patches. The motivation is unclear: lazy afternoon? :P
Martin von Zweigbergk wrote:
> [...]
> diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
> index aea6685..d7c874c 100755
> --- a/t/t3401-rebase-partial.sh
> +++ b/t/t3401-rebase-partial.sh
> @@ -11,51 +11,50 @@ local branch.
> '
> . ./test-lib.sh
>
> -test_expect_success \
> - 'prepare repository with topic branch' \
> - 'echo First > A &&
> - git update-index --add A &&
> - git commit -m "Add A." &&
> +test_expect_success 'prepare repository with topic branch' '
> + echo First > A &&
> + git update-index --add A &&
> + git commit -m "Add A." &&
Style nit: >[^ ] is prevalent FWIW.
$ git grep '> [^ ]' t/ | wc -l
3091
$ git grep '>[^ ]' t/ | wc -l
9271
Sure, the regular expressions aren't tailored to make sure that only
redirections are caught, but I suppose it's safe to assume that one
number is significantly larger than the other.
> [...]
> -test_expect_success \
> - 'rebase topic branch against new master and check git am did not get halted' \
> - 'git rebase master && test ! -d .git/rebase-apply'
> +test_expect_success 'rebase topic branch against new master and check git am did not get halted' '
> + git rebase master &&
> + test ! -d .git/rebase-apply
> +'
While at it, why not change this "test ! -d" to
"test_path_is_missing"? My rationale is that you're touching the file
anyway to do a generic cleanup; might as well finish it.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Ramkumar Ramachandra @ 2011-12-09 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <7v8vmlfuw1.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> In testing, a common paradigm involves checking the expected output
>> with the actual output: test-lib provides a test_cmp to show the diff
>> between the two outputs. So, use this function in preference to
>> calling a human over to compare command outputs by eye.
>>
>> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>> t/t1006-cat-file.sh | 119 ++++++++++++++++++++++++---------------------------
>> 1 files changed, 56 insertions(+), 63 deletions(-)
>>
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index d8b7f2f..5be3ce9 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
>>
>> run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
>>
>> -test_expect_success \
>> - "Reach a blob from a tag pointing to it" \
>> - "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
>> +test_expect_success "Reach a blob from a tag pointing to it" '
>> + echo_without_newline "$hello_content" >expect &&
>> + git cat-file blob "$tag_sha1" >actual &&
>> + test_cmp expect actual
>> +'
>>
>> +test_done
>> for batch in batch batch-check
>> do
>> for opt in t s e p
>
> What is that test_done about?
:facepalm: crept in while testing -- I often (temporarily) use a
test_done after the test I want to inspect so the others aren't
unnecessarily executed hiding the error.
Thanks for catching this.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Junio C Hamano @ 2011-12-09 18:43 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Jonathan Nieder, Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> In testing, a common paradigm involves checking the expected output
> with the actual output: test-lib provides a test_cmp to show the diff
> between the two outputs. So, use this function in preference to
> calling a human over to compare command outputs by eye.
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> t/t1006-cat-file.sh | 119 ++++++++++++++++++++++++---------------------------
> 1 files changed, 56 insertions(+), 63 deletions(-)
>
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index d8b7f2f..5be3ce9 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
>
> run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
>
> -test_expect_success \
> - "Reach a blob from a tag pointing to it" \
> - "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
> +test_expect_success "Reach a blob from a tag pointing to it" '
> + echo_without_newline "$hello_content" >expect &&
> + git cat-file blob "$tag_sha1" >actual &&
> + test_cmp expect actual
> +'
>
> +test_done
> for batch in batch batch-check
> do
> for opt in t s e p
What is that test_done about?
^ permalink raw reply
* Re: git auto-repack is broken...
From: Nicolas Pitre @ 2011-12-09 18:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Casey, Jeff King, Linus Torvalds,
Ævar Arnfjörð, Git Mailing List
In-Reply-To: <7v4nx9hcmv.fsf@alter.siamese.dyndns.org>
On Fri, 9 Dec 2011, Junio C Hamano wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
>
> > On Sat, 3 Dec 2011, Brandon Casey wrote:
> >
> >> Linus's scenario of fetching a lot of stuff that never actually makes
> >> it into the reflogs is still a valid problem. I'm not sure that
> >> people who don't know what they are doing are going to run into this
> >> problem though. Since he fetches a lot of stuff without ever checking
> >> it out or creating a branch from it, potentially many objects become
> >> unreferenced every time FETCH_HEAD changes.
> >
> > Maybe FETCH_HEAD should have a reflog too?
>
> It is a feature that the objects that were fetched for a quick peek become
> immediately unreferenced and eligible for early removal unless they are
> kept somewhere, e.g. remote tracking refs. What problem are we trying to
> solve?
This is indeed a tangential observation to the expiration delay. I was
just suggesting that having a reflog for FETCH_HEAD in the case when you
fetch a branch with an explicit URL might be handy.
Nicolas
^ permalink raw reply
* Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style
From: Ramkumar Ramachandra @ 2011-12-09 18:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <7vd3bxfvob.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>> Right, fixed.
>
> Does the "fixed" mean "I fixed it locally; please expect to see it in the
> next re-roll?"
Yes, it means exactly that :)
I respond/ archive the review emails as soon as I fix the
corresponding issue locally -- it's the only way to make sure that I
don't miss anything (although I miss a few comments despite that).
-- Ram
^ permalink raw reply
* Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style
From: Junio C Hamano @ 2011-12-09 18:26 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <CALkWK0nbp465915ysrBXi46mur1dutBDtPNjwW=RdyPV03crzg@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> I suspect the above description, while it does describe your patch,
>> does not describe the _reason_ that the patch exists or that someone
>> would want to apply it. Isn't it something more like:
>> [...]
>
> Right, fixed.
Does the "fixed" mean "I fixed it locally; please expect to see it in the
next re-roll?"
Just a quick question, not asking you to always spell it that way.
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 18:00 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111209022913.GA2600@sigill.intra.peff.net>
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.
^ permalink raw reply
* Re: Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 17:50 UTC (permalink / raw)
To: Jakub Narebski, git
In-Reply-To: <m3zkf1hnh9.fsf@localhost.localdomain>
On Dec 9, 2011, at 8:41 AM, Jakub Narebski wrote:
> Sidney San Martín <s@sidneysm.com> writes:
>
>> *Nothing else* in my everyday life works this way anymore. Line
>> wrapping gets done on the display end in my email client, my web
>> browser, my ebook reader entirely automatically, and it adapts to
>> the size of the window.
>
> The problem with automatic wrapping on the display is that there could
> be parts of commit message that *shouldn't* be wrapped, like code
> sample, or URL... and in plain text you don't have a way to separate
> flowed from non-flowed part.
>
I usually lead code, URLs, and other preformatted lines like this with a few spaces. Markdown uses the same convention, and it looks like commits in this repo do too.
The patch in my last email prints lines which begin with whitespace verbatim. How does that work?
> Also with long non-breakable identifiers you sometimes need to wrap by
> hand (or use linebreaking algorithm from TeX) or go bit over the limit
> to make it look nice.
>
Could you elaborate on this? The patch uses strbuf_add_wrapped_text, which was already in Git. If an identifier is longer than the wrap width, it leaves it alone.
> BTW. proper editor used to create commit message can wrap it for you
> ;-).
Only if everybody else in the world does it too! And only if I never care about seeing my commits at any width besides 80 columns.
> --
> Jakub Narębski
>
^ 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