* Re: [RFC/PATCH v2] CodingGuidelines: add Python coding guidelines
From: Pete Wyckoff @ 2013-02-03 15:12 UTC (permalink / raw)
To: John Keeping; +Cc: Michael Haggerty, git
In-Reply-To: <20130201111634.GA2476@farnsworth.metanate.com>
john@keeping.me.uk wrote on Fri, 01 Feb 2013 11:16 +0000:
> On Fri, Feb 01, 2013 at 09:39:39AM +0100, Michael Haggerty wrote:
> > On 01/30/2013 09:31 PM, John Keeping wrote:
> > > On Wed, Jan 30, 2013 at 11:05:10AM +0100, Michael Haggerty wrote:
> > >> [...] maybe we should establish a small Python library of
> > >> compatibility utilities (like a small "six"). [...]
> > >> But I haven't had time to think of where to put such a library, how to
> > >> install it, etc.
> > >
> > > If we want to go that route, I think restructuring the
> > > "git_remote_helpers" directory and re-using its infrastructure for
> > > installing the "Git Python modules" would be the way to go. The
> > > directory structure would become something like this:
> > >
> > > git/
> > > `-- python/
> > > |-- Makefile # existing file pulled out of git_remote_helpers
> > > |-- < some new utility library >
> > > |-- git_remote_helpers
> > > | |-- __init__.py
> > > | |-- git
> > > | | |-- __init__.py
> > > | | |-- exporter.py
> > > | | |-- git.py
> > > | | |-- importer.py
> > > | | |-- non_local.py
> > > | | `-- repo.py
> > > | `-- util.py
> > > |-- setup.cfg # existing file pulled out of git_remote_helpers
> > > `-- setup.py # existing file pulled out of git_remote_helpers
> > >
> > >
> > > It looks like the GitPython project[1] as already taken the "git" module
> > > name, so perhaps we should use "git_core" if we do introduce a new
> > > module.
> > >
> > > [1] http://pypi.python.org/pypi/GitPython
> >
> > This sounds reasonable. But not all Python code will go under the
> > "python" subdirectory, right? For example, I am working on a Python
> > script that fits thematically under contrib/hooks.
>
> I was thinking of it as analagous with the "perl" directory that
> currently exists. So the "python" directory will contain library code
> but scripts can live wherever is most appropriate.
>
> One way of looking at it is: could the user want to have this installed
> for all available versions of Python? For a script, the answer is "no"
> because they will call it and it will just run. For libraries, you want
> them to be available with whatever Python interpreter you happen to be
> running (assuming that it is a version supported by the library).
>
> > OTOH (I'm thinking aloud here) it is probably a bad idea for a hook
> > script to depend on a Python module that is part of git itself. Doing
> > so would make the hook script depend on a particular version of git (or
> > at least a version with a compatible Python module). But users might be
> > reluctant to upgrade git just to install a hook script.
>
> I don't think such a dependency is a bad idea in the longer term. If a
> "Git Python library" is developed, then at some point most people who
> have Git installed will have some version of that library - it becomes a
> case of perhaps wanting to limit yourself to some subset of the library
> rather than just not using it.
>
> In fact, git_remote_helpers has been available since Git 1.7.0 and
> contains a lot of functionality that is more generic than its name
> suggests.
This library idea would be a great help; there are 100-odd calls
to git in git-p4, and we've had to deal with getting the arguments
and parsing correct. I'd happily switch to using git_core.
Probably some elements of GitPython can be used too. I'm not so
interested in the raw database manipulation, but the command
wrappers look reasonable.
-- Pete
^ permalink raw reply
* Re: Feature request: Allow extracting revisions into directories
From: Sitaram Chamarty @ 2013-02-03 16:25 UTC (permalink / raw)
To: Robert Clausecker; +Cc: git
In-Reply-To: <1359901085.24730.11.camel@t520>
On 02/03/2013 07:48 PM, Robert Clausecker wrote:
> Hello!
>
> git currently has the archive command that allows to save an arbitrary
> revision into a tar or zip file. Sometimes it is useful to not save this
> revision into an archive but to directly put all files into an arbitrary
> directory. Currently this seems to be not possible to archive directly;
> the only way I found to do it is to run git archive and then directly
> unpack the archive into a directory.
>
> git --git-dir REPO archive REVISION | tar x
>
> It would be nice to have a command or simply a switch to git archive
> that allows the user to put the files of REVISION into a directory
> instead of making an archive.
Could you help me understand why piping it to tar (actually 'tar -C
/dest/dir -x') is not sufficient to achieve what you want?
^ permalink raw reply
* Re: [PATCH 0/3] Make Git compile warning-free with Clang
From: Antoine Pelisse @ 2013-02-03 17:13 UTC (permalink / raw)
To: John Keeping; +Cc: git
In-Reply-To: <cover.1359901732.git.john@keeping.me.uk>
Thanks John,
I couldn't find any time to send that "sum-up" series.
On Sun, Feb 3, 2013 at 3:37 PM, John Keeping <john@keeping.me.uk> wrote:
> The first two patches here were sent to the list before but seem to have
> got lost in the noise [1][2]. The final one is new but was prompted by
> discussion in the same thread.
^ permalink raw reply
* Re: [PATCH] send-email: ignore files ending with ~
From: Antoine Pelisse @ 2013-02-03 17:16 UTC (permalink / raw)
To: Alexandre Courbot; +Cc: Junio C Hamano, git
In-Reply-To: <1359903340-14508-1-git-send-email-gnurou@gmail.com>
On Sun, Feb 3, 2013 at 3:55 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> It certainly happened to a lot of people already: you carefully prepare
> your set of patches, export them using format-patch --cover-letter,
> write your cover letter, and send the set like this:
>
> $ git send-email --to=somerenowneddeveloper --to=myfutureemployer
> --cc=thismailinglistiwanttoimpress 00*
Why don't you use 00*.patch ? That seems dubious to me to ignore files
specified on the command line.
^ permalink raw reply
* Re: Feature request: Allow extracting revisions into directories
From: Robert Clausecker @ 2013-02-03 18:11 UTC (permalink / raw)
To: git
In-Reply-To: <510E8F82.9050306@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 757 bytes --]
Am Sonntag, den 03.02.2013, 21:55 +0530 schrieb Sitaram Chamarty:
> Could you help me understand why piping it to tar (actually 'tar -C
> /dest/dir -x') is not sufficient to achieve what you want?
Piping the output of git archive into tar is of course a possible
solution; I just don't like the fact that you need to pipe the output
into a separate program to do something that should be possible with a
simple switch and not an extra command. It feels unintuitive and like a
workaround to make an archive just to unpack it on-the-fly. Also, adding
such a command (or at least documenting the way to do this using a pipe
to tar somewhere in the man pages) is a small and simple change that
improves usability.
Yours, Robert Clausecker
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: Tay Ray Chuan @ 2013-02-03 18:20 UTC (permalink / raw)
To: John Keeping; +Cc: Git Mailing List, Antoine Pelisse
In-Reply-To: <6995fd5e4d9cb3320ab80c983f1b25ae8a399284.1359901732.git.john@keeping.me.uk>
On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:
> When compiling combine-diff.c, clang 3.2 says:
>
> combine-diff.c:1006:19: warning: adding 'int' to a string does not
> append to the string [-Wstring-plus-int]
> prefix = COLONS + offset;
> ~~~~~~~^~~~~~~~
> combine-diff.c:1006:19: note: use array indexing to silence this warning
> prefix = COLONS + offset;
> ^
> & [ ]
>
> Suppress this by making the suggested change.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> combine-diff.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index bb1cc96..dba4748 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
> offset = strlen(COLONS) - num_parent;
> if (offset < 0)
> offset = 0;
> - prefix = COLONS + offset;
> + prefix = &COLONS[offset];
>
> /* Show the modes */
> for (i = 0; i < num_parent; i++) {
> --
Hmm, does
prefix = (const char *) COLONS + offset;
suppress the warning?
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [PATCH] gitk: Display the date of a tag in a human friendly way.
From: Anand Kumria @ 2013-02-03 18:37 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Junio C Hamano, git
In-Reply-To: <7v1udzqv1v.fsf@alter.siamese.dyndns.org>
Hi Pau,
I've not been able to find the canonical location of your gitk repository.
I've tried kernel.org, samba.org and ozlabs.org; none of them to have
it - nor does any amount of google searching I do reveal the location.
I realise you've probably had a busy month with linux.conf.au but it
would be nice to have some feedback.
Could you let me know where gitk is hosted and I'll re-roll this patch
against that (and update the docs so others don't need to go hunting).
Thanks,
Anand
On 5 January 2013 19:38, Junio C Hamano <gitster@pobox.com> wrote:
> Anand Kumria <wildfire@progsoc.org> writes:
>
>> Sorry, I didn't know that gitk had been split back out (and
>> Documentation/gitk.txt still mentions it is part of the git suite).
>
> It is not "split back" at all, and it won't be. From "git" user's
> point of view it is part of the suite.
>
> Gitk however is still a viable freestanding project, so it would be
> selfish for me to take a patch to gitk-git/gitk directly to my tree,
> as the patch will not be able to flow back to the standalone gitk
> project. Hence we always let patches go through Paul's tree and then
> I pull from him.
>
>
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: John Keeping @ 2013-02-03 19:06 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: Git Mailing List, Antoine Pelisse
In-Reply-To: <CALUzUxowrh53g50ZxkXSjLfOrSgX-YiZEB2MJXbLwxmwNB187A@mail.gmail.com>
On Mon, Feb 04, 2013 at 02:20:06AM +0800, Tay Ray Chuan wrote:
> On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:
> > When compiling combine-diff.c, clang 3.2 says:
> >
> > combine-diff.c:1006:19: warning: adding 'int' to a string does not
> > append to the string [-Wstring-plus-int]
> > prefix = COLONS + offset;
> > ~~~~~~~^~~~~~~~
> > combine-diff.c:1006:19: note: use array indexing to silence this warning
> > prefix = COLONS + offset;
> > ^
> > & [ ]
> >
> > Suppress this by making the suggested change.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> > combine-diff.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/combine-diff.c b/combine-diff.c
> > index bb1cc96..dba4748 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
> > offset = strlen(COLONS) - num_parent;
> > if (offset < 0)
> > offset = 0;
> > - prefix = COLONS + offset;
> > + prefix = &COLONS[offset];
> >
> > /* Show the modes */
> > for (i = 0; i < num_parent; i++) {
>
> Hmm, does
>
> prefix = (const char *) COLONS + offset;
>
> suppress the warning?
It does, but it turns out that the following also suppresses the
warning:
-- >8 --
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..a07d329 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,7 +982,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
free(sline);
}
-#define COLONS "::::::::::::::::::::::::::::::::"
+static const char COLONS[] = "::::::::::::::::::::::::::::::::";
static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
{
I think that's a nicer change than the original suggestion.
John
^ permalink raw reply related
* Re: [PATCH 1/3] fix clang -Wtautological-compare with unsigned enum
From: Jonathan Nieder @ 2013-02-03 19:38 UTC (permalink / raw)
To: John Keeping; +Cc: git, Antoine Pelisse
In-Reply-To: <a9fe675ed9b34d3c15f4678ee13e90cddaa36055.1359901732.git.john@keeping.me.uk>
John Keeping wrote:
> From: Antoine Pelisse <apelisse@gmail.com>
>
> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> sane and silent the clang warning.
Thanks. Looks good to me.
[...]
> --- a/grep.c
> +++ b/grep.c
> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
> for (p = opt->header_list; p; p = p->next) {
> if (p->token != GREP_PATTERN_HEAD)
> die("bug: a non-header pattern in grep header list.");
> - if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> + if (p->field < GREP_HEADER_FIELD_MIN ||
> + GREP_HEADER_FIELD_MAX <= p->field)
> die("bug: unknown header field %d", p->field);
I also think it would be fine to drop this test or replace it with an
assert((unsigned) p->field < ARRAY_SIZE(header_field));
because we know the test never trips.
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Jeff King @ 2013-02-03 19:41 UTC (permalink / raw)
To: git
In-Reply-To: <87k3qrx712.fsf@lifelogs.com>
On Sat, Feb 02, 2013 at 06:57:29AM -0500, Ted Zlatanov wrote:
> I wrote a Perl credential helper for netrc parsing which is pretty
> robust, has built-in docs with -h, and doesn't depend on external
> modules. The netrc parser regex was stolen from Net::Netrc.
>
> It will by default use ~/.authinfo.gpg, ~/.netrc.gpg, ~/.authinfo, and
> ~/.netrc (whichever is found first) and this can be overridden with -f.
Cool, thanks for working on this.
> If the file name ends with ".gpg", it will run "gpg --decrypt FILE" and
> use the output. So non-interactively, that could hang if GPG was
> waiting for input. Does Git handle that, or should I check for a TTY?
No, git does not do anything special with respect to credential helpers
and ttys (nor should it, since one use of helpers is to get credentials
when there is no tty). I think it is GPG's problem to deal with, though.
We will invoke it, and it is up to it to decide whether it can acquire
the passphrase or not (either through the tty, or possibly from
gpg-agent). So it would be wrong to do the tty check yourself.
I haven't tested GPG, but I assume it properly tries to read from
/dev/tty and not stdin. Your helper's stdio is connected to git and
speaking the credential-helper protocol, so GPG reading from stdin would
either steal your input (if run before you read it), or just get EOF (if
you have read all of the pipe content already). If GPG isn't well
behaved, it may be worth redirecting its stdin from /dev/null as a
safety measure.
> Take a look at the proposed patch and let me know if it's usable, if you
> need a formal copyright assignment, etc.
Overall looks sane to me, though my knowledge of .netrc is not
especially good. Usually we try to send patches inline in the email
(i.e., as generated by git-format-patch), and include a "Signed-off-by"
line indicating that content is released to the project; see
Documentation/SubmittingPatches.
> +use Data::Dumper;
I don't see it used here. Leftover from debugging?
> + print <<EOHIPPUS;
Cute, I haven't seen that one before.
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: any use is OK.
I don't know if we have a particular policy for items in contrib/, but
this license may be too vague. In particular, it does not explicitly
allow redistribution, which would make Junio shipping a release with it
a copyright violation.
Any objection to just putting it under some well-known simple license
(GPL, BSD, or whatever)?
> +if ($file =~ m/\.gpg$/)
> +{
> + $file = "gpg --decrypt $file|";
> +}
Does this need to quote $file, since the result will get passed to the
shell? It might be easier to just use the list form of open(), like:
my @data = $file =~ /\.gpg$/ ?
load('-|', qw(gpg --decrypt), $file) :
load('<', $file);
(and then obviously update load to just dump all of @_ to open()).
> +die "Sorry, we could not load data from [$file]"
> + unless (scalar @data);
Probably not that interesting a corner case, but this means we die on an
empty .netrc, whereas it might be more sensible for it to behave as "no
match".
For the same reason, it might be worth silently exiting when we don't
find a .netrc (or any of its variants). That lets people who share their
dot-files across machines configure git globally, even if they don't
necessarily have a netrc on every machine.
> +# the query
> +my %q;
> +
> +foreach my $v (values %{$options{tmap}})
> +{
> + undef $q{$v};
> +}
Just my personal style, but I find the intent more obvious with "map" (I
know some people find it unreadable, though):
my %q = map { $_ => undef } values(%{$options{tmap}});
> +while (<STDIN>)
> +{
> + next unless m/([a-z]+)=(.+)/;
We don't currently have any exotic tokens that this would not match, nor
do I plan to add them, but the credential documentation defines a valid
line as /^([^=]+)=(.+)/.
It's also possible for the value to be empty, but I do not think
off-hand that current git will ever send such an empty value.
> [...]
The rest of it looks fine to me. I don't think any of my comments are
show-stoppers. Tests would be nice, but integrating contrib/ stuff with
the test harness is kind of a pain.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2013, #01; Fri, 1)
From: Junio C Hamano @ 2013-02-03 19:43 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130203130237.GS1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Sun, Feb 03, 2013 at 04:13:22AM -0800, David Aguilar wrote:
>> It looks good to go. The additional "|| :" in the makefile is a nice
>> touchup that made it more robust too.
>
> Looks good to me as well.
Thanks, both.
^ permalink raw reply
* Re: Getting started contributing.
From: Junio C Hamano @ 2013-02-03 19:46 UTC (permalink / raw)
To: Duy Nguyen; +Cc: adamfraser, git
In-Reply-To: <CACsJy8AYOAwLKufQ34brk1agyFAX9xjgAE9_LAcRx=RGxcEZzg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, Feb 3, 2013 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> On the other hand, there probably still are many loose ends.
>
> A few other things
>
> - Mark more strings for translation (not as easy as it sounds, some
> strings can't be translated)
True, but not a good advice for somebody new, exactly for the reason
you stated, i.e. some strings must not be translated.
> - Color more in the output where it makes sense
Eeek.
> - Stop/Warn the user from updating HEAD (e.g. checkout another
> branch) while in the middle of a rebase (some makes sense, most is by
> mistake)
Perhaps, but again probably not for somebody new who hasn't mastered
various workflows and understood why it may make sense to allow it.
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: Junio C Hamano @ 2013-02-03 19:58 UTC (permalink / raw)
To: John Keeping; +Cc: git, Antoine Pelisse
In-Reply-To: <6995fd5e4d9cb3320ab80c983f1b25ae8a399284.1359901732.git.john@keeping.me.uk>
John Keeping <john@keeping.me.uk> writes:
> When compiling combine-diff.c, clang 3.2 says:
>
> combine-diff.c:1006:19: warning: adding 'int' to a string does not
> append to the string [-Wstring-plus-int]
> prefix = COLONS + offset;
> ~~~~~~~^~~~~~~~
> combine-diff.c:1006:19: note: use array indexing to silence this warning
> prefix = COLONS + offset;
> ^
> & [ ]
>
> Suppress this by making the suggested change.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
This was not lost in the noise.
I thought that this wasn't a serious patch, but your attempt to
demonstrate to others why patches trying to squelch clang warnings
are not necessarily a good thing to do.
Who is that compiler trying to help with such a warning message?
After all, we are writing in C, and clang is supposed to be a C
compiler. And adding integer to a pointer to (const) char is a
straight-forward way to look at the trailing part of a given string.
> - prefix = COLONS + offset;
> + prefix = &COLONS[offset];
In other words, both are perfectly valid C. Why should we make it
less readable to avoid a stupid compiler warning?
^ permalink raw reply
* Re: [PATCH] Handle path completion and colon for tcsh script
From: Manlio Perillo @ 2013-02-03 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Marc Khouzam, git@vger.kernel.org
In-Reply-To: <7vboc2bhpj.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 02/02/2013 21:10, Junio C Hamano ha scritto:
> Marc Khouzam <marc.khouzam@ericsson.com> writes:
>
>> Recent enhancements to git-completion.bash provide
>> intelligent path completion for git commands. Such
>> completions do not add the '/' at the end of directories
>> for recent versions of bash.
>> ...
>> Here is the update for tcsh completion which is needed to handle
>> the cool new path completion feature just pushed to 'next'.
>>
> [...]
> But I have to wonder if this is sweeping a problem under the rug.
> Shouldn't the completion for bash users end completed directory name
> with '/', even if we didn't have to worry about tcsh?
>
The problem is that when using the "new"
`compopt -o filenames` command, Bash assumes COMPREPLY contains a list
of filenames, and when it detects a directory name, it adds a slash.
The problem is, if the directory name *already* has a slash, Bash adds
another slash!
I don't know if this can be considered a bug or a feature.
Regards Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlEOwaQACgkQscQJ24LbaUSjwgCfbgb1id5DcNG0Q75FWwgNPCqb
qkUAnAmMzCahB745/BWeDJTHbJFXucxs
=vf+P
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] gitk: Display the date of a tag in a human friendly way.
From: Jonathan Nieder @ 2013-02-03 20:12 UTC (permalink / raw)
To: Anand Kumria; +Cc: Paul Mackerras, Junio C Hamano, git
In-Reply-To: <CAM1C4G=mKzfgrfO-n_yXjyDkZ9vA7H6kQEY5Yja-5C-YRRLmyw@mail.gmail.com>
Hi Anand,
Anand Kumria wrote:
> I've not been able to find the canonical location of your gitk repository.
Here's how I find it:
$ git clone git://repo.or.cz/git.git
[...]
$ cd git
$ git log -1 --oneline -- gitk-git
ec3ae6ec Merge git://ozlabs.org/~paulus/gitk
$ cd ..
$ git clone git://ozlabs.org/~paulus/gitk.git
Patches, including documentation patches, go to git@vger.kernel.org,
cc-ing Paul Mackerras.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH] gitk: Display the date of a tag in a human friendly way.
From: Junio C Hamano @ 2013-02-03 20:23 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Anand Kumria, Paul Mackerras, git
In-Reply-To: <20130203201217.GB3221@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Anand Kumria wrote:
>
>> I've not been able to find the canonical location of your gitk repository.
>
> Here's how I find it:
>
> $ git clone git://repo.or.cz/git.git
> [...]
> $ cd git
> $ git log -1 --oneline -- gitk-git
> ec3ae6ec Merge git://ozlabs.org/~paulus/gitk
> $ cd ..
> $ git clone git://ozlabs.org/~paulus/gitk.git
>
> Patches, including documentation patches, go to git@vger.kernel.org,
> cc-ing Paul Mackerras.
Or look at "A note from the maintainer" posted here every few
months.
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: John Keeping @ 2013-02-03 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Antoine Pelisse
In-Reply-To: <7vwqup890o.fsf@alter.siamese.dyndns.org>
On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > When compiling combine-diff.c, clang 3.2 says:
> >
> > combine-diff.c:1006:19: warning: adding 'int' to a string does not
> > append to the string [-Wstring-plus-int]
> > prefix = COLONS + offset;
> > ~~~~~~~^~~~~~~~
> > combine-diff.c:1006:19: note: use array indexing to silence this warning
> > prefix = COLONS + offset;
> > ^
> > & [ ]
> >
> > Suppress this by making the suggested change.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
>
> This was not lost in the noise.
>
> I thought that this wasn't a serious patch, but your attempt to
> demonstrate to others why patches trying to squelch clang warnings
> are not necessarily a good thing to do.
>
> Who is that compiler trying to help with such a warning message?
> After all, we are writing in C, and clang is supposed to be a C
> compiler. And adding integer to a pointer to (const) char is a
> straight-forward way to look at the trailing part of a given string.
A quick search turned up the original thread where this feature was
added to Clang [1]. It seems that it does find genuine bugs where
people try to log values by doing:
log("failed to handle error: " + errno);
[1] http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203
> > - prefix = COLONS + offset;
> > + prefix = &COLONS[offset];
>
> In other words, both are perfectly valid C. Why should we make it
> less readable to avoid a stupid compiler warning?
Are you happy to change COLONS to a const char[] instead of a #define?
That also suppresses the warning.
Since Git is warning-free on GCC and so close to being warning-free on
recent Clang I think it is worthwhile to fix the remaining two issues
which do seem to be intentional diagnostics rather than Clang bugs.
John
^ permalink raw reply
* Re: [PATCH] Handle path completion and colon for tcsh script
From: Junio C Hamano @ 2013-02-03 20:43 UTC (permalink / raw)
To: Manlio Perillo; +Cc: Marc Khouzam, git@vger.kernel.org
In-Reply-To: <510EC1A4.4000207@gmail.com>
Manlio Perillo <manlio.perillo@gmail.com> writes:
> The problem is that when using the "new"
> `compopt -o filenames` command, Bash assumes COMPREPLY contains a list
> of filenames, and when it detects a directory name, it adds a slash.
>
> The problem is, if the directory name *already* has a slash, Bash adds
> another slash!
So bash users do see the trailing slash because bash adds one to
what we compute and return, which we do strip the trailing slash
exactly because we know bash will add one. Because tcsh completion
uses what we compute directly, without bash massaging our output to
add the trailing slash, it needs some magic.
OK, that makes sense. It was this part from the originally proposed
log message:
>> ... Such completions do not add the '/' at the end of directories
>> for recent versions of bash. However, the '/' is needed by tcsh,
>> ...
with a large gap between the two sentences that fooled me, and the
explanation in your message helped to fill the gap to understand the
situation better.
Perhaps
... for recent versions of bash, which will then add the
trailing slash for paths that are directory to the result of
our completion. The completion for tcsh however uses the
result of our completion directly, so it either needs to add
the necessary slash itself, or needs to ask us to keep the
trailiing slash. This patch does the latter.
or something?
^ permalink raw reply
* Re: [PATCH 3/3] builtin/apply: tighten (dis)similarity index parsing
From: Junio C Hamano @ 2013-02-03 20:50 UTC (permalink / raw)
To: John Keeping; +Cc: git, Antoine Pelisse
In-Reply-To: <2cac21192f79f9fbb5822417775954eba29064fa.1359901732.git.john@keeping.me.uk>
John Keeping <john@keeping.me.uk> writes:
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 6c11e8b..4745e75 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)
>
> static int gitdiff_similarity(const char *line, struct patch *patch)
> {
> - if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
> - patch->score = 0;
> + unsigned long val = strtoul(line, NULL, 10);
> + if (val <= 100)
> + patch->score = val;
> return 0;
> }
>
> static int gitdiff_dissimilarity(const char *line, struct patch *patch)
> {
> - if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
> - patch->score = 0;
> + unsigned long val = strtoul(line, NULL, 10);
> + if (val <= 100)
> + patch->score = val;
> return 0;
> }
This makes sort of sense; .score is used only for display and not
for making any decision, so as long as you know it is initialized to
zero when the call to this function is made, it should be OK.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: Junio C Hamano @ 2013-02-03 21:07 UTC (permalink / raw)
To: John Keeping; +Cc: git, Antoine Pelisse
In-Reply-To: <20130203203150.GU1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> A quick search turned up the original thread where this feature was
> added to Clang [1]. It seems that it does find genuine bugs where
> people try to log values by doing:
>
> log("failed to handle error: " + errno);
To be perfectly honest, anybody who writes such a code should be
sent back to school before trying to touch out code ever again ;-).
It is not even valid Python, Perl nor Java, I would think.
> Are you happy to change COLONS to a const char[] instead of a #define?
Happy? Not really.
It could be a good change for entirely different reason. We will
save space if we ever need to use it in multiple places. But the
entire "COLONS + offset" thing was a hack we did, knowing that it
will break when we end up showing a muiti-way diff for more than 32
blobs.
If we were to be touching that area of code, I'd rather see a change
to make it more robust against such a corner case. If it results in
squelching misguided clang warnings against programmers who should
not be writing in C, that is a nice side effect, but I loathe to see
any change whose primary purpose is to squelch pointless warnings.
combine-diff.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..7f6187f 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
free(sline);
}
-#define COLONS "::::::::::::::::::::::::::::::::"
-
static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
{
struct diff_options *opt = &rev->diffopt;
- int i, offset;
- const char *prefix;
- int line_termination, inter_name_termination;
+ int line_termination, inter_name_termination, i;
line_termination = opt->line_termination;
inter_name_termination = '\t';
@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
show_log(rev);
if (opt->output_format & DIFF_FORMAT_RAW) {
- offset = strlen(COLONS) - num_parent;
- if (offset < 0)
- offset = 0;
- prefix = COLONS + offset;
+ /* As many colons as there are parents */
+ for (i = 0; i < num_parent; i++)
+ putchar(':');
/* Show the modes */
- for (i = 0; i < num_parent; i++) {
- printf("%s%06o", prefix, p->parent[i].mode);
- prefix = " ";
- }
- printf("%s%06o", prefix, p->mode);
+ for (i = 0; i < num_parent; i++)
+ printf("%06o ", p->parent[i].mode);
+ printf("%06o", p->mode);
/* Show sha1's */
for (i = 0; i < num_parent; i++)
^ permalink raw reply related
* Re: [PATCH v2] branch: show rebase/bisect info when possible instead of "(no branch)"
From: Matthieu Moy @ 2013-02-03 21:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Niedier
In-Reply-To: <1359870520-22644-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -164,7 +164,7 @@ test_expect_success 'bisect start: existing ".git/BISECT_START" not modified if
> cp .git/BISECT_START saved &&
> test_must_fail git bisect start $HASH4 foo -- &&
> git branch > branch.output &&
> - test_i18ngrep "* (no branch)" branch.output > /dev/null &&
> + test_i18ngrep "* (bisecting other)" branch.output > /dev/null &&
I'd have spelled it (no branch, bisecting other) to make it clear that
we're on detached HEAD, and avoid confusing old-timers. But maybe your
version is enough, I'm not sure.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2] branch: show rebase/bisect info when possible instead of "(no branch)"
From: Junio C Hamano @ 2013-02-03 21:58 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Nguyễn Thái Ngọc Duy, git, Jonathan Niedier
In-Reply-To: <vpqpq0hnlb1.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> --- a/t/t6030-bisect-porcelain.sh
>> +++ b/t/t6030-bisect-porcelain.sh
>> @@ -164,7 +164,7 @@ test_expect_success 'bisect start: existing ".git/BISECT_START" not modified if
>> cp .git/BISECT_START saved &&
>> test_must_fail git bisect start $HASH4 foo -- &&
>> git branch > branch.output &&
>> - test_i18ngrep "* (no branch)" branch.output > /dev/null &&
>> + test_i18ngrep "* (bisecting other)" branch.output > /dev/null &&
>
> I'd have spelled it (no branch, bisecting other) to make it clear that
> we're on detached HEAD, and avoid confusing old-timers. But maybe your
> version is enough, I'm not sure.
Yeah, I do not think "bisecting other" alone makes much sense.
What does "other" refer to when you start your bisection at a
detached head? I personally think "other" has _any_ value in that
message, because "(no branch, bisecting)" gives the same amount of
information, especially because "other" does not say which branch it
refers to at all.
^ permalink raw reply
* Re: [PATCH v2] branch: show rebase/bisect info when possible instead of "(no branch)"
From: Junio C Hamano @ 2013-02-03 22:00 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Nguyễn Thái Ngọc Duy, git, Jonathan Niedier
In-Reply-To: <7v1ucx83fz.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Yeah, I do not think "bisecting other" alone makes much sense.
>
> What does "other" refer to when you start your bisection at a
> detached head? I personally think "other" has _any_ value in that
s/_any_/_no_/; obviously ;-)
> message, because "(no branch, bisecting)" gives the same amount of
> information, especially because "other" does not say which branch it
> refers to at all.
^ permalink raw reply
* [New Feature] git-submodule-move - Easily move submodules
From: TJ @ 2013-02-03 22:36 UTC (permalink / raw)
To: git
I've recently had need to re-arrange more than ten submodules within a project and discovered there is apparently no easy way to do it.
Using some suggestions I found on Stack Overflow I eventually figured out the steps required. Because the steps can be
complex I thought it would be handy to have a tool to automate the functionality.
I have put together a reasonably bullet-proof shell script "git-submodule-move" that does the job pretty well. I've put it through quite a bit of testing and trusted it with my own project and it has
performed well.
I've published to github so others can use and improve it.
https://github.com/iam-TJ/git-submodule-move
^ permalink raw reply
* Re: [PATCH] git p4: chdir resolves symlinks only for relative paths
From: Pete Wyckoff @ 2013-02-03 23:08 UTC (permalink / raw)
To: Miklós Fazekas; +Cc: git, Gary Gibbons
In-Reply-To: <CAAMmcSSEzs3+vZDO=FDMV9c2rp-8HTdMuPeeQCkok6y7sRDYJw@mail.gmail.com>
mfazekas@szemafor.com wrote on Tue, 29 Jan 2013 09:37 +0100:
> If a p4 client is configured to /p/foo which is a symlink
> to /vol/bar/projects/foo, then resolving symlink, which
> is done by git-p4's chdir will confuse p4: "Path
> /vol/bar/projects/foo/... is not under client root /p/foo"
> While AltRoots in p4 client specification can be used as a
> workaround on p4 side, git-p4 should not resolve symlinks
> in client paths.
> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve
> relative paths, but as a side effect it resolves symlinks
> too. Now it checks if the dir is relative before resolving.
>
> Signed-off-by: Miklós Fazekas <mfazekas@szemafor.com>
> ---
> git-p4.py | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 2da5649..5d74649 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -64,7 +64,10 @@ def chdir(dir):
> # not using the shell, we have to set it ourselves. This path could
> # be relative, so go there first, then figure out where we ended up.
> os.chdir(dir)
> - os.environ['PWD'] = os.getcwd()
> + if os.path.isabs(dir):
> + os.environ['PWD'] = dir
> + else:
> + os.environ['PWD'] = os.getcwd()
>
> def die(msg):
> if verbose:
Thanks, this is indeed a bug and I have reproduced it with a test
case. Your patch works, but I think it would be better to
separate the callers of chdir(): those that know they are
cd-ing to a path from a p4 client, and everybody else. The former
should not use os.getcwd(), as you show.
I'll whip something up soon, unless you beat me to it.
-- Pete
^ 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