* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Kevin Ballard @ 2010-12-07 20:41 UTC (permalink / raw)
To: Scott Kyle; +Cc: Ævar Arnfjörð Bjarmason, git
In-Reply-To: <AANLkTinuD2ze_mn5QMLqFaoftwZvTsz-DKv1ojAizg7P@mail.gmail.com>
On Dec 7, 2010, at 12:37 PM, Scott Kyle wrote:
> On Tue, Dec 7, 2010 at 4:15 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Tue, Dec 7, 2010 at 00:22, Scott Kyle <scott@appden.com> wrote:
>>> For those who often work on repositories with submodules, the dirty
>>> indicator for unstaged changes will almost always show because development
>>> is simultaneously happening on those submodules. The config option
>>> diff.ignoreSubmodules is not appropriate for this use because it has larger
>>> implications.
>>
>> Wouldn't it be a lot better to instead add support for showing
>> submodule dirtyness as distinct from the main tree's dirtyness? Then
>> you could easily spot if you had either your tree / submodule tree
>> changes, without just ignoring them.
>
> I considered that, but thought it to be a rather disruptive change,
> and one that conceptually didn't work. The way I see it, either
> somebody thinks of their repo as dirty when the submodules are dirty,
> or not. And I think since this behavior has perpetuated for so long,
> most users are content with how it currently works. I, however, was
> not, and so that is why I added an option for people like me.
The big win for such a change, from my perspective, is it tells me if I need
to do a `git submodule update --recursive`, or if I actually have dirty changes.
Because of that, if nobody else picks this up, I'll probably write a patch
to introduce such a config at some point in the future. But as I said before,
that's something that can be done later and doesn't need to affect this patch.
-Kevin Ballard
^ permalink raw reply
* Re: [PATCH] logging branch deletion to help recovering from mistakes
From: Jonathan Nieder @ 2010-12-07 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Casey Dahlin, git
In-Reply-To: <7vfwu9pbyj.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> "git log -g HEAD"
> keeps track of what was at the tip of HEAD, be it pointing at a branch or
> pointing diretly at a commit in a detached state, no?
Yes.
1. Imagine I have an interesting branch and delete it:
$ git branch interesting $(lots of hard work)
$ git branch -D interesting
Oops. If I want to recover that branch, I may have a lot of digging
to do in the HEAD reflog. It may not be there are all. Your patch
mitigates that by allowing a simple "I didn't mean that" command.
$ git branch --undelete interesting
2. Great. Another way to lose a line of development, as Casey
mentioned, is to not give it a branch name in the first place:
$ git checkout HEAD^0
...
$ git checkout something-else
Oops. Well, not so bad. If I want to recover my old work, I can
simply use
$ git checkout HEAD@{1}
3. Now suppose I was not paying attention and made the mistake
from (1) or (2) a week ago and didn't realize it. Now I want to
get back that code.
If it was situation (1), I can remember the name of the branch
and do
$ git branch --undelete interesting
No problem [1]. If it was situation (2), I need to dig through the
HEAD reflog. As Jeff explained, it is possible to script something up
to help organize the search. I think Casey was suggesting doing that
work at HEAD-reattachment time instead, so you could do
$ git branch --undelete-detached-head=old-head
to recover the last line of development made without a branch;
my response was that if this ends up frequently being useful
then I suspect something is wrong with the workflow.
Hoping that is clearer,
Jonathan
[1] as long as the branch name was not reused
^ permalink raw reply
* Re: [PATCH] Improved error messages when temporary file creation fails
From: Junio C Hamano @ 2010-12-07 20:56 UTC (permalink / raw)
To: Arnout Engelen; +Cc: git
In-Reply-To: <20101207181633.GF25767@bzzt.net>
Arnout Engelen <arnouten@bzzt.net> writes:
> This patch has been submitted/discussed before, but that version doesn't apply
> cleanly to the newest git anymore, so here's an updated version.
Thanks, but that comes after "---", no?
> It improves diagnostic error messages when creating a temporary file fails.
>
> Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
> ---
> diff --git a/wrapper.c b/wrapper.c
> index 4c1639f..6640c87 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
> * Various trivial helper wrappers around standard functions
> */
> #include "cache.h"
> +#include "wrapper.h"
>
> static void do_nothing(size_t size)
> {
> @@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)
> int xmkstemp(char *template)
> {
> int fd;
> + char origtemplate[255];
> + strlcpy(origtemplate, template, 255);
Why "255"?
It may happen to be sufficiently large for the current callers, but what
provisions if any are made to help the compiler or the runtime protect us
from new and broken callers? Use of strlcpy() there hides the issue from
the runtime by avoiding segfault, but it actively harms us by making the
code silently behave incorrectly without segfaulting, no?
> diff --git a/wrapper.h b/wrapper.h
> new file mode 100644
> index 0000000..b06ff0d
> --- /dev/null
> +++ b/wrapper.h
> @@ -0,0 +1,4 @@
> +/*
> + * Various trivial helper wrappers around standard functions
> + */
> +int xmkstemp(char *template);
Somewhat questionable...
Why doesn't this say "extern"?
What happend to another copy of this line in git-compat-util.h? IOW, what
protects this and the other one from drifting apart?
Why doesn't users include wrapper.h but only wrapper.c?
Why yet another header is needed only for this function and nothing else?
Why isn't this protected with the standard #ifndef .../#define .../#endif?
^ permalink raw reply
* Re: [RFC/PATCH 2/1] bash: eliminate dependency on bash_completion lib
From: Jonathan Nieder @ 2010-12-07 20:59 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Peter van der Does, Shawn O. Pearce, git, Marc Branchaud,
Brian Gernhardt, Kevin Ballard, Mathias Lafeldt
In-Reply-To: <20101207204104.GE1867@neumann>
SZEDER Gábor wrote:
> On Tue, Dec 07, 2010 at 01:49:23PM -0600, Jonathan Nieder wrote:
>> Ah, sorry, applies to d93f4a297 (bash: work around bash 4.0 change in
>> COMP_WORDS semantics, 2010-12-02).
>
> In which repo? ;)
Thanks. I had meant to say that patch 1 applies to 06f44c3c
(completion: make compatible with zsh, 2010-09-06) and that the bash 4
support could be rebased to work without that if there is demand.
*goes off to get some coffee*
Sorry for the confusion.
Jonathan
^ permalink raw reply
* Re: [RFC/PATCH 2/1] bash: eliminate dependency on bash_completion lib
From: Junio C Hamano @ 2010-12-07 21:03 UTC (permalink / raw)
To: Jonathan Nieder
Cc: SZEDER Gábor, Peter van der Does, Shawn O. Pearce, git,
Marc Branchaud, Brian Gernhardt, Kevin Ballard, Mathias Lafeldt
In-Reply-To: <20101207194923.GD22587@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> SZEDER Gábor wrote:
>> On Thu, Dec 02, 2010 at 03:02:07PM -0600, Jonathan Nieder wrote:
>
>>> Add a minimal implementation of _get_comp_words_by_ref,
>>> the routine used to work around bash 4.0's COMP_WORDS semantics.
>>>
>>> Based on bash-completion 2.x (commit bf763033, 2010-10-26) but
>>> tweaked for simplicity and to allow zsh to at least parse the
>>> code.
>>
>> What is this patch based on?
>
> Ah, sorry, applies to d93f4a297 (bash: work around bash 4.0 change in
> COMP_WORDS semantics, 2010-12-02).
Sorry, but whose repository does that object live in?
^ permalink raw reply
* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Jens Lehmann @ 2010-12-07 21:08 UTC (permalink / raw)
To: Kevin Ballard; +Cc: Ævar Arnfjörð Bjarmason, Scott Kyle, git
In-Reply-To: <0E479F18-B26A-4216-A71E-C65EAB41A74A@sb.org>
Am 07.12.2010 21:31, schrieb Kevin Ballard:
> On Dec 7, 2010, at 4:15 AM, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Dec 7, 2010 at 00:22, Scott Kyle <scott@appden.com> wrote:
>>> For those who often work on repositories with submodules, the dirty
>>> indicator for unstaged changes will almost always show because development
>>> is simultaneously happening on those submodules. The config option
>>> diff.ignoreSubmodules is not appropriate for this use because it has larger
>>> implications.
>>
>> Wouldn't it be a lot better to instead add support for showing
>> submodule dirtyness as distinct from the main tree's dirtyness? Then
>> you could easily spot if you had either your tree / submodule tree
>> changes, without just ignoring them.
>
> That sounds like a good idea, but it doesn't necessarily have to come with
> this patch. Scott's use case here is he has a submodule that is _always_ dirty,
> and he simply doesn't want to see that stuff in the PS1. Having an option to
> show it separately would be very useful for me, but should perhaps be written
> as a separate patch.
I'm not sure if I understand your case correctly, but if there is only one
submodule that is always dirty and everybody knows that but nobody cares,
won't it make sense to change the "submodule.<name>.ignore" config option
for that peculiar submodule via .git/config or .gitmodules?
^ permalink raw reply
* how to make a commit only contain existing files
From: david @ 2010-12-07 21:00 UTC (permalink / raw)
To: git
I know that I can do a git rm to explictly remove files, but is there an
easy way to just say that this commit should contain all the files that
exist at this point in time, without carrying over any files that were in
a prior commit but that don't exist now?
David Lang
^ permalink raw reply
* Re: how to make a commit only contain existing files
From: Jeff King @ 2010-12-07 21:11 UTC (permalink / raw)
To: david; +Cc: git
In-Reply-To: <alpine.DEB.2.00.1012071203190.662@asgard.lang.hm>
On Tue, Dec 07, 2010 at 01:00:18PM -0800, david@lang.hm wrote:
> I know that I can do a git rm to explictly remove files, but is there
> an easy way to just say that this commit should contain all the files
> that exist at this point in time, without carrying over any files
> that were in a prior commit but that don't exist now?
Won't "git add -A; git commit" do what you want?
-Peff
^ permalink raw reply
* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Scott Kyle @ 2010-12-07 21:17 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Kevin Ballard, Ævar Arnfjörð Bjarmason, git
In-Reply-To: <4CFEA249.907@web.de>
On Tue, Dec 7, 2010 at 1:08 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 07.12.2010 21:31, schrieb Kevin Ballard:
>> On Dec 7, 2010, at 4:15 AM, Ęvar Arnfjörš Bjarmason wrote:
>>
>>> On Tue, Dec 7, 2010 at 00:22, Scott Kyle <scott@appden.com> wrote:
>>>> For those who often work on repositories with submodules, the dirty
>>>> indicator for unstaged changes will almost always show because development
>>>> is simultaneously happening on those submodules. The config option
>>>> diff.ignoreSubmodules is not appropriate for this use because it has larger
>>>> implications.
>>>
>>> Wouldn't it be a lot better to instead add support for showing
>>> submodule dirtyness as distinct from the main tree's dirtyness? Then
>>> you could easily spot if you had either your tree / submodule tree
>>> changes, without just ignoring them.
>>
>> That sounds like a good idea, but it doesn't necessarily have to come with
>> this patch. Scott's use case here is he has a submodule that is _always_ dirty,
>> and he simply doesn't want to see that stuff in the PS1. Having an option to
>> show it separately would be very useful for me, but should perhaps be written
>> as a separate patch.
>
> I'm not sure if I understand your case correctly, but if there is only one
> submodule that is always dirty and everybody knows that but nobody cares,
> won't it make sense to change the "submodule.<name>.ignore" config option
> for that peculiar submodule via .git/config or .gitmodules?
>
If I set the "submodule.<name>.ignore" then diffing around inside my
history will not show the changes to that particular submodule. That
is what I meant by diff.ignoreSubmodules having larger implications.
^ permalink raw reply
* Re: [PATCH] Improved error messages when temporary file creation fails
From: Arnout Engelen @ 2010-12-07 21:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v62v5paj2.fsf@alter.siamese.dyndns.org>
Thanks to you and Jonathan again for the feedback.
On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
> > + char origtemplate[255];
> > + strlcpy(origtemplate, template, 255);
>
> Why "255"?
Random - 'i had to choose something'.
> It may happen to be sufficiently large for the current callers, but what
> provisions if any are made to help the compiler or the runtime protect us
> from new and broken callers? Use of strlcpy() there hides the issue from
> the runtime by avoiding segfault, but it actively harms us by making the
> code silently behave incorrectly without segfaulting, no?
Only in a small way: when a bigger template is encountered and the mkstemp
call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
template, the diagnostic error message shows a truncated version of the
original.
I *could* dynamically allocate space for the original template string, but that
would mean I'd need to do a malloc() instead of putting the buffer on the stack
like this, and free() it afterwards. I'm not too concerned about the
performance hit here (presumably the I/O that comes with creating and using
the temporary file here is orders of magnitude slower than that malloc()
anyway), but it would also make the code a bit less easy to read.
What do you think would be preferable here, a simple fixed-length buffer on the
stack that might cause a truncated error message or a dynamically-allocated
one that makes the code somewhat more complicated?
> > +++ b/wrapper.h
>
> Somewhat questionable...
Agreed, this whole file is unneeded and, well, wrong anyway.
I'll remove wrapper.h and apply Jonathan's improvements some time this week,
unless of course someone beats me to it :).
Kind regards,
Arnout
^ permalink raw reply
* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Jens Lehmann @ 2010-12-07 21:28 UTC (permalink / raw)
To: Scott Kyle; +Cc: Kevin Ballard, Ævar Arnfjörð Bjarmason, git
In-Reply-To: <AANLkTinnH4pFaEf=e4YE64f7cwLRx2R_2o_-=JGua30b@mail.gmail.com>
Am 07.12.2010 22:17, schrieb Scott Kyle:
> On Tue, Dec 7, 2010 at 1:08 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> Am 07.12.2010 21:31, schrieb Kevin Ballard:
>>> On Dec 7, 2010, at 4:15 AM, Ęvar Arnfjörš Bjarmason wrote:
>>>
>>>> On Tue, Dec 7, 2010 at 00:22, Scott Kyle <scott@appden.com> wrote:
>>>>> For those who often work on repositories with submodules, the dirty
>>>>> indicator for unstaged changes will almost always show because development
>>>>> is simultaneously happening on those submodules. The config option
>>>>> diff.ignoreSubmodules is not appropriate for this use because it has larger
>>>>> implications.
>>>>
>>>> Wouldn't it be a lot better to instead add support for showing
>>>> submodule dirtyness as distinct from the main tree's dirtyness? Then
>>>> you could easily spot if you had either your tree / submodule tree
>>>> changes, without just ignoring them.
>>>
>>> That sounds like a good idea, but it doesn't necessarily have to come with
>>> this patch. Scott's use case here is he has a submodule that is _always_ dirty,
>>> and he simply doesn't want to see that stuff in the PS1. Having an option to
>>> show it separately would be very useful for me, but should perhaps be written
>>> as a separate patch.
>>
>> I'm not sure if I understand your case correctly, but if there is only one
>> submodule that is always dirty and everybody knows that but nobody cares,
>> won't it make sense to change the "submodule.<name>.ignore" config option
>> for that peculiar submodule via .git/config or .gitmodules?
>>
>
> If I set the "submodule.<name>.ignore" then diffing around inside my
> history will not show the changes to that particular submodule. That
> is what I meant by diff.ignoreSubmodules having larger implications.
Ah, seems I misunderstood your submodule being dirty as modified or
untracked files being present in it's work tree. But your submodules
HEAD seems to differ from the commit recorded in the superproject,
and then of course "submodule.<name>.ignore=dirty" won't help you.
^ permalink raw reply
* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Jonathan Nieder @ 2010-12-07 21:29 UTC (permalink / raw)
To: Scott Kyle
Cc: Jens Lehmann, Kevin Ballard,
Ævar Arnfjörð Bjarmason, git
In-Reply-To: <AANLkTinnH4pFaEf=e4YE64f7cwLRx2R_2o_-=JGua30b@mail.gmail.com>
Scott Kyle wrote:
> If I set the "submodule.<name>.ignore" then diffing around inside my
> history will not show the changes to that particular submodule.
Even if you set it to "dirty"?
^ permalink raw reply
* git format-patch should honor notes
From: Eric Blake @ 2010-12-07 21:53 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]
I'm just starting to experiment with 'git notes', because it seems to
fit well with my workflow on several projects, except for one drawback.
My workflow is that I post patch series for upstream review via 'git
send-email'. Often, that results in feedback that requires me to
amend/rebase my series, and post a v2 or v3 of the series. By adding
'git config notes.rewriteRef refs/notes/commits', I can add notes that
will carry across my rebase, and remind me what I changed in v2 (for
example, git notes add -m 'v2: fix foo, per mail xyz@example.com').
This is handy for me, and I think it is also handy for reviewers -
someone who took the time to read through v1 should know what I changed
in response to their comments, and only have to focus in on commits with
changes, rather than on the entire resent series.
However, I think such review helps are informational only - that is, in
'git send-email' parlance, they belong between the '-- ' and diffstat
lines of the email, and not in the upstream commit. After all, once my
series is finally accepted upstream, it will no longer be rebased, and
'git bisect' sees only the final version. I see no reason for the
commit message to carry the cruft of extra information that was only
helpful during reviewing the amended series, nor any reason why upstream
should carry around my notes.
So, what I'm missing is the ability for 'git send-email' (or more
fundamentally, 'git format-patch') to be able to include contents of a
particular (set of) notes reference in each patch file it generates,
where the note falls in the informative portion of the email, and is
intentionally omitted from the upstream commit when someone else runs
'git am' on my email.
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] describe: Use for_each_rawref
From: Junio C Hamano @ 2010-12-07 21:59 UTC (permalink / raw)
To: Anders Kaseorg
Cc: SZEDER Gábor, Jonathan Nieder, git, Kirill Smelkov,
Thomas Rast, Shawn O. Pearce, Mark Levedahl, Jens Lehmann
In-Reply-To: <7vsjy9pdmg.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Anders Kaseorg <andersk@ksplice.com> writes:
>
>> On Tue, 7 Dec 2010, Anders Kaseorg wrote:
>>> Signed-off-by: Anders Kaseorg <andersk@ksplice.com>
>>>
>>> diff --git a/builtin/describe.c b/builtin/describe.c
>>
>> (Gaaah, sorry, I accidentally used format-patch -p here. Won’t happen
>> again. :-) )
>
> The default will work just fine, no?
>
> Also labelling them like "[PATCH v2 1/2]" would have been nicer to spot
> the progress of the series.
Two more requests.
* Please describe what was changed since earlier interations after "---"
lines;
* Please run tests before submitting patches. It appears to break t7407.
What breaks in t7407 is "git submodule status --recursive" output. In my
environment, it used to describe the tip as heads/master but after the
patch it says remotes/origin/master. The output from "describe --all"
does not tiebreak between one branch from another, even though it does
favor tags over branches (and tries to use annotated ones), so it is not a
real regression in that sense, but then the test needs to be fixed not to
care about which equally good branches are shown.
Some might even argue that it is more relevant to show how the commit
relates to remote-tracking branch heads. That is a separate issue (if
this position is widely supported, we would introduce another "prio"
between "all others (0)" and "lightweight tags (1)" for remote-tracking
branches), though.
^ permalink raw reply
* Re: git format-patch should honor notes
From: Junio C Hamano @ 2010-12-07 22:10 UTC (permalink / raw)
To: Eric Blake; +Cc: Git Mailing List
In-Reply-To: <4CFEACC5.70005@redhat.com>
Eric Blake <eblake@redhat.com> writes:
> So, what I'm missing is the ability for 'git send-email' (or more
> fundamentally, 'git format-patch') to be able to include contents of a
> particular (set of) notes reference in each patch file it generates,
> where the note falls in the informative portion of the email, and is
> intentionally omitted from the upstream commit when someone else runs
> 'git am' on my email.
I do not know if "should" is the right word, but it certainly sounds like
it would be nice to have such an option for the usecase you described.
^ permalink raw reply
* Re: git format-patch should honor notes
From: Jeff King @ 2010-12-07 22:11 UTC (permalink / raw)
To: Eric Blake; +Cc: Thomas Rast, Michael J Gruber, Git Mailing List
In-Reply-To: <4CFEACC5.70005@redhat.com>
On Tue, Dec 07, 2010 at 02:53:09PM -0700, Eric Blake wrote:
> My workflow is that I post patch series for upstream review via 'git
> send-email'. Often, that results in feedback that requires me to
> amend/rebase my series, and post a v2 or v3 of the series. By adding
> 'git config notes.rewriteRef refs/notes/commits', I can add notes that
> will carry across my rebase, and remind me what I changed in v2 (for
> example, git notes add -m 'v2: fix foo, per mail xyz@example.com').
> This is handy for me, and I think it is also handy for reviewers -
> someone who took the time to read through v1 should know what I changed
> in response to their comments, and only have to focus in on commits with
> changes, rather than on the entire resent series.
Yeah, that is a workflow that some others have mentioned using here,
too. And I think there is general agreement that notes should go after
the "---" in format-patch. We just need a working patch.
Thomas posted one in February:
http://article.gmane.org/gmane.comp.version-control.git/140819
But there were some issues and it never got polished. Michael suggested
that he does something similar here:
http://article.gmane.org/gmane.comp.version-control.git/140819
but there was no indication on whether it happens manually or if he has
a patch. I don't know if anything else has happened in that area. I'm
sure if you feel like working on a patch it would be well received.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] Re: git submodule -b ... of current HEAD fails
From: Junio C Hamano @ 2010-12-07 22:57 UTC (permalink / raw)
To: Jens Lehmann
Cc: Jonathan Nieder, git, Klaus Ethgen, Sven Verdoolaege, mlevedahl
In-Reply-To: <4CF80B71.3010309@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Nope, these lines date back to the time before I got involved in the
> submodule business ... Seems like this "git checkout" was added in
> March 2008 by Mark Levedahl (CCed), maybe he can shed some light on
> that.
>
> But to me your change looks good, so feel free to add:
> Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
Does either of you want to add a test for this?
^ permalink raw reply
* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Scott Kyle @ 2010-12-07 22:59 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jens Lehmann, Kevin Ballard, Ævar Arnfjörð, git
In-Reply-To: <20101207212949.GA25162@burratino>
On Tue, Dec 7, 2010 at 1:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Scott Kyle wrote:
>
>> If I set the "submodule.<name>.ignore" then diffing around inside my
>> history will not show the changes to that particular submodule.
>
> Even if you set it to "dirty"?
>
Setting it to "dirty" is far less disruptive, you're right, but that
wouldn't do me much good since my submodules are often on different
branches while developing.
^ permalink raw reply
* Re: [PATCHv3 5/6] web--browse: use *www-browser if available
From: Junio C Hamano @ 2010-12-07 23:36 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Christian Couder, Jonathan Nieder
In-Reply-To: <1291657790-3719-6-git-send-email-giuseppe.bilotta@gmail.com>
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> + # if the linked executable doesn't match a browser name we know about,
> + # look at the version string
> +
> + # even though most browsers (and applications, in fact) will show their
> + # name and version on the first line of the --version output, this is
> + # not true in particular for the KDE apps (e.g. konqueror and kfmclient),
> + # which display their name and version on the LAST line. So we cannot
> + # clip the version string at the first line when retrieving it. Rather,
> + # we keep it whole and then limit it when we know what we're dealing with.
I'd be more worried about the ones that do not understand --version and
spawn a new window.
> + verstring="$("$testexe" --version 2> /dev/null)"
> + browser="$(echo "$verstring" | head -n 1 | cut -f1 -d' ' | tr A-Z a-z)"
> + case "$browser" in
> + mozilla)
What was the first patch in this series about again ;-)?
> + verstring="$(echo "$verstring" | head -n 1)"
> + browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
> + ;;
> + google)
> + verstring="$(echo "$verstring" | head -n 1)"
> + browser="google-chrome"
> + ;;
> + qt:)
> + # konqueror, kfmclient or other KDE app
> + verstring="$(echo "$verstring" | tail -n 1)"
> + browser="$(echo "$verstring" | cut -f1 -d:)"
> + ;;
> + *)
> + verstring="$(echo "$verstring" | head -n 1)"
> + ;;
> +
> + esac
> + if valid_tool "$browser" ; then
> + browser_path="$i"
> + return 0
> + fi
> +
> + echo >&2 "$testexe (detected as $browser) is not a supported browser, skipping"
> + browser=""
> + return 1
> +}
Sorry, but I simply do not think it is worth this ugliness to get slight
customization between -new-tab, newTab, and nothingness.
^ permalink raw reply
* Re: [PATCHv2 2/7] web--browse: coding style
From: Junio C Hamano @ 2010-12-07 23:38 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Christian Couder, Jonathan Nieder
In-Reply-To: <1291394861-11989-3-git-send-email-giuseppe.bilotta@gmail.com>
Thanks for a patch that is easy to verify.
"git show -w" gives an empty result ;-)
^ permalink raw reply
* Re: [PATCHv3 4/6] web--browse: better support for chromium
From: Junio C Hamano @ 2010-12-07 23:38 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Christian Couder, Jonathan Nieder
In-Reply-To: <1291657790-3719-5-git-send-email-giuseppe.bilotta@gmail.com>
Up to this point of the series, I think v3 is a regression compared to
what is queued on 'pu'.
^ permalink raw reply
* Re: [PATCH] Improved error messages when temporary file creation fails
From: Jakub Narebski @ 2010-12-07 23:56 UTC (permalink / raw)
To: git
In-Reply-To: <20101207212041.GG25767@bzzt.net>
Arnout Engelen wrote:
> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
>>> + char origtemplate[255];
>>> + strlcpy(origtemplate, template, 255);
>>
>> Why "255"?
>
> Random - 'i had to choose something'.
>
>> It may happen to be sufficiently large for the current callers, but what
>> provisions if any are made to help the compiler or the runtime protect us
>> from new and broken callers? Use of strlcpy() there hides the issue from
>> the runtime by avoiding segfault, but it actively harms us by making the
>> code silently behave incorrectly without segfaulting, no?
>
> Only in a small way: when a bigger template is encountered and the mkstemp
> call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
> template, the diagnostic error message shows a truncated version of the
> original.
>
> I *could* dynamically allocate space for the original template string, but that
> would mean I'd need to do a malloc() instead of putting the buffer on the stack
> like this, and free() it afterwards. I'm not too concerned about the
> performance hit here (presumably the I/O that comes with creating and using
> the temporary file here is orders of magnitude slower than that malloc()
> anyway), but it would also make the code a bit less easy to read.
>
> What do you think would be preferable here, a simple fixed-length buffer on the
> stack that might cause a truncated error message or a dynamically-allocated
> one that makes the code somewhat more complicated?
Why not use PATH_MAX instead of 255?
P.S. I'm sory for cutting up CC list...
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] Improved error messages when temporary file creation fails
From: Jonathan Nieder @ 2010-12-08 0:12 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Arnout Engelen, Junio C Hamano
In-Reply-To: <idmhjl$n1j$1@dough.gmane.org>
Jakub Narebski wrote:
> Arnout Engelen wrote:
>> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
>>>> + char origtemplate[255];
>>>> + strlcpy(origtemplate, template, 255);
>>>
>>> Why "255"?
[...]
> Why not use PATH_MAX instead of 255?
The advantage I can see to 256 is a small speed-up in the "no errors"
code path. Since the I/O would tend to be much more costly and PATH_MAX
is self explanatory, using PATH_MAX does seem cleaner.
I know you all are aware of this already; just thought I'd mention it
while forwarding the original message to Arnout. :)
Jonathan
^ permalink raw reply
* Re: [PATCH 01/14] msvc: Fix compilation errors in compat/win32/sys/poll.c
From: Ramsay Jones @ 2010-12-07 22:54 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Johannes Sixt, kusmabite, GIT Mailing-list
In-Reply-To: <20101204204541.GA3170@burratino>
Jonathan Nieder wrote:
> Could it make sense to define this at the same time as _GNU_SOURCE et
> al?
>
> #define _ALL_SOURCE 1
> #define _GNU_SOURCE 1
> #define _BSD_SOURCE 1
> #define _NETBSD_SOURCE 1
> #define _SGI_SOURCE 1
> #define _WIN32_WINNT 0x0502
>
> Haven't thought carefully about the consequences, though; your patch
> is probably safer.
[Sorry for the late reply, I've been away from email for several days]
This would not fix the compilation errors, since compat/win32/sys/poll.c
does not include the git-compat-util.h header file (and I *don't* think
it should). ;-)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 04/14] msvc: Fix macro redefinition warnings
From: Ramsay Jones @ 2010-12-08 0:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, GIT Mailing-list, sschuberth
In-Reply-To: <201012042204.52002.j6t@kdbg.org>
Johannes Sixt wrote:
> On Samstag, 4. Dezember 2010, Ramsay Jones wrote:
>> --- a/compat/mingw.h
>> +++ b/compat/mingw.h
>> @@ -14,12 +14,6 @@ typedef int socklen_t;
>> #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
>> #define S_ISSOCK(x) 0
>>
>> -#ifndef _STAT_H_
>
> Instead of removing the macros, wouldn't we be much safer with just
>
> #ifndef S_IWUSR
>
> ? ...
Er... no.
Commit 4091bfc (which added these macros) does not provide any motivation
for the change, and I'm having a hard time trying to imagine a useful
purpose for this part of the commit. (I'm not saying there isn't one - just
that I can't see it :-P )
On MinGW, the <sys/stat.h> header is always included prior to this header,
so the _STAT_H_ include guard is always defined, so these macros will
never be defined (which is a *good* thing; have you looked at the definitions).
Trying to use compat/mingw.h without having first included <sys/stat.h> is
not going to work!
Also, note that the include guard for the msvc <sys/stat.h> file is _INC_STAT.
So, on msvc, including <sys/stat.h> does not suppress these macro definitions
(Not that it actually matters here, because it doesn't define these symbols
anyway!). Which is why msvc issues these macro redefinition warnings (they
conflict with the definitions in compat/vcbuild/include/unistd.h). We most
definitely don't want to use the macros in compat/mingw.h on msvc. (They are
positively *wrong*)
[Hmmm, I've just noticed that the msvc compat header is missing a definition of
the _S_IRWXU macro!]
So, once again, I see no reason to keep them ... Unless you know otherwise.
ATB,
Ramsay Jones
^ 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