* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Junio C Hamano @ 2011-12-09 17:23 UTC (permalink / raw)
To: Frans Klaver; +Cc: git
In-Reply-To: <op.v56xbxqs0aolir@keputer>
"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.
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?
^ permalink raw reply
* Re: git auto-repack is broken...
From: Junio C Hamano @ 2011-12-09 17:35 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Brandon Casey, Jeff King, Linus Torvalds,
Ævar Arnfjörð, Git Mailing List
In-Reply-To: <alpine.LFD.2.02.1112071709250.2907@xanadu.home>
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?
I thought everybody agreed that the current expire window for unreachable
objects is way too conservative, especially given that the only purpose of
that window is to protect live objects from concurrent gcs. Perhaps the
only thing we need to do is to trim that window down to say 2 days or even
8 hours?
^ permalink raw reply
* Re: Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 17:49 UTC (permalink / raw)
To: Frans Klaver; +Cc: git
In-Reply-To: <CAH6sp9OCbR_NG-sPn7Eq2d4LYbJAbaPX6rCdZR6rbet_nyaGCA@mail.gmail.com>
Ah, thank you!
On Dec 9, 2011, at 11:49 AM, Frans Klaver wrote:
> I'm adding git@vger... again, cause there didn't seem to be a reason not to.
>
> On Fri, Dec 9, 2011 at 3:10 PM, Sidney San Martín <s@sidneysm.com> wrote:
>> On Dec 9, 2011, at 2:05 AM, Frans Klaver wrote:
>>
>>> On Fri, 09 Dec 2011 02:59:06 +0100, Sidney San Martín <s@sidneysm.com> wrote:
>>>
>>>> Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters.
>>>>
>>>> The webpage most cited about it, which I otherwise really like, is
>>>>
>>>> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>>>>
>>>> *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.
>>>
>>> Actually, opera-mail autowraps at 72 characters but sets the text format to flowed. It also wraps the quoted text when you reply. But there's a reasonable chance that you don't use opera in your daily life. On the other hand I would not be surprised if most decent e-mail clients worked that way.
>>>
>>
>> Interesting… either way, the end result is that the receiving mail client can wrap the lines to whatever length it (or you, as its operator) desires, which I think we can agree is a good thing, right?
>>
>
> Yes.
>
>>> Hm. Saying "that's how the tool works" is not a good reason in my opinion. There might be tons of other reasons for wrapping at 80 characters. Readability is one that comes to mind for me.
>>>
>>
>> That's my basic point. I hope it didn't seem like I was arguing against reading commit messages wrapped to 80 columns, by default. I only wanted to discuss whether it makes more sense to handle it on the display end instead of asking committers to do it in advance.
>>
>
> It somewhat looked like that. I think it might make sense for clients
> to ignore the line wrapping when they can only show less than 80
> characters on a line, but that would probably break the code part of a
> patch mail.
>
>
>> - My phone shows text most comfortably at about 40 characters per line. I do look at terminals at 80 columns most of the time, but not always, and I sometimes browse projects in GUI tools that use a proportional font in a window may be narrower or wider than that.
>>
>> - Right now, when I *am* in an 80-col terminal I have to trust everyone else to wrap their commit messages. Not everyone does. I feel like it would be more effective to give git the ability to wrap them automatically when I read them.
>>
>
> It could be a useful option to wrap when the lines extend the window
> width, but I'd actually think it's better to leave that up to the
> pager than to git.
>
>>>>
>>>> Second:
>>>>
>>>>> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.)
>>>>
>>>> There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself.
>>>
>>> Yes, that standard allows e-mail clients to display the text more fluidly, even if the source text is word-wrapped. While git uses e-mail format, it isn't an e-mail client. I always interpreted this whole thing as git basically creating plain-text e-mails. You're actually writing the source of the e-mail in your commit message. If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails. That said, Apple Mail (the client you used to send your mail) doesn't even use the RFC you quote in the sent message. That mail is going to be a pain in the butt to read in mutt from work ;).
>>>
>>
>> Sorry, I'm not sure what you mean by, “If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails”.
>
> I thought you might want to have wrapped text in the git commit
> messages, but actually put a format flowed tag into the mail header.
> I'm not sure what that would do to the code though.
>
>
>> Interesting, I didn't realize that Mail didn't use it. It does, however, use quoted-printable which, as far as I can tell, has a similar effect on line wrapping. What happens when you view this email in mutt?
>>
>
> I had no idea quoted printable had any effect on line wrapping. As far
> as I know it's just a way to encode non-ascii characters in 7bit, no
> more no less. Your current e-mail happens to end lines with =<nl>,
> which probably handles the wrapping. Your original message didn't have
> that.
>
>
>>>> - - -
>>>>
>>>> From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
>>>> Date: Thu, 8 Dec 2011 20:26:23 -0500
>>>> Subject: [PATCH] Wrap commit messages on display
>>>>
>>>> - Wrap to 80 characters minus the indent
>>>> - Use a hanging indent for lines which begin with "- "
>>>> - Do not wrap lines which begin with whitespace
>>>> ---
>>>> pretty.c | 10 ++++++++--
>>>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/pretty.c b/pretty.c
>>>> index 230fe1c..15804ce 100644
>>>> --- a/pretty.c
>>>> +++ b/pretty.c
>>>> @@ -1243,8 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
>>>> memset(sb->buf + sb->len, ' ', indent);
>>>> strbuf_setlen(sb, sb->len + indent);
>>>> }
>>>> - strbuf_add(sb, line, linelen);
>>>> - strbuf_addch(sb, '\n');
>>>> + if (line[0] == ' ' || line[0] == '\t') {
>>>> + strbuf_add(sb, line, linelen);
>>>> + } else {
>>>> + struct strbuf wrapped = STRBUF_INIT;
>>>> + strbuf_add(&wrapped, line, linelen);
>>>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent);
>>>
>>> While on the subject, In my mail view, the new line started with the [1] from line[1], in the quote the line looks entirely different. Now this is code we're talking about, so it makes slightly more sense to have a proper wrapping hard-coded. Compare the above with the following:
>>>
>>> + int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
>>> [...]
>>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0,
>>> + indent + hanging_indent,
>>> + 80 - indent);
>>>
>>> Much clearer, no? I personally usually have two or three terminals tucked next to each other, so I can look at two or three things at the same time. 80 characters limit is a nice feature then.
>>
>> Good point, that makes it clearer either way. I put an updated patch at the bottom of this email (also fixed forgetting the newline after lines with leading whitespace). I hope it's OK to include patches this way, I understand that they're supposed to represent whole emails but want to include them with this discussion.
>>
>
> You can include them in the discussion. While it is probably OK to put
> some code into your mail to propose something (I've seen it happen
> more than once), the end result is supposed to be submitted with a
> git-format-patch'd commit. You can read more about contributing in
> Documentation/SubmittingPatches.
>
>
>>>
>>>
>>>> + strbuf_addch(sb, '\n');
>>>> + }
>>>> }
>>>> }
>>>>
>>>
>>> Cheers,
>>> Frans
>>
>>
>> From 53fd7deedaf5ac522c9d752e79cf71561cc57f07 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
>> Date: Thu, 8 Dec 2011 20:26:23 -0500
>> Subject: [PATCH] Wrap commit messages on display
>>
>> - Wrap to 80 characters, minus the indent
>> - Use a hanging indent for lines which begin with "- "
>> - Do not wrap lines which begin with whitespace
>> ---
>> pretty.c | 9 ++++++++-
>> 1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/pretty.c b/pretty.c
>> index 230fe1c..841ccd1 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1243,7 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
>> memset(sb->buf + sb->len, ' ', indent);
>> strbuf_setlen(sb, sb->len + indent);
>> }
>> - strbuf_add(sb, line, linelen);
>> + if (line[0] == ' ' || line[0] == '\t') {
>> + strbuf_add(sb, line, linelen);
>> + } else {
>> + struct strbuf wrapped = STRBUF_INIT;
>> + strbuf_add(&wrapped, line, linelen);
>> + int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + hanging_indent, 80 - indent);
>
> It's common in C (and in certain flavors even required) to have your
> variable declaration at the beginning of the scope:
>
> + } else {
> + int hanging_indent;
> + struct strbuf wrapped = STRBUF_INIT;
> + strbuf_add(&wrapped, line, linelen);
> + hanging_indent = ((line[0] == '-' && line[1]
> == ' ') ? 2 : 0);
> + strbuf_add_wrapped_text(sb, wrapped.buf, 0,
> indent + hanging_indent, 80 - indent);
>
>
> Gmail webclient mucks up the whitespace. Don't copy & paste ;)
>
>
> As I said earlier in the mail, I'm not sure if this is something that
> should be done by git. Maybe someone else can shed some light on that.
>
>
>> + }
>> strbuf_addch(sb, '\n');
>> }
>> }
>> --
>> 1.7.8
>>
>>
^ 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
* 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: [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: [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: 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 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: [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 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 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 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 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] 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: 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 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 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 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: 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 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 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 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 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 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
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