* Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings
From: Joachim Schmitz @ 2013-03-21 13:44 UTC (permalink / raw)
To: git
In-Reply-To: <514AF2E1.7020409@viscovery.net>
Johannes Sixt wrote:
> Am 3/21/2013 12:03, schrieb Jeff King:
>> I was fooling around with clang and noticed that it complains about
>> the "int x = x" construct under -Wall. That is IMHO a deficiency in
>> clang, since the idiom has a well-defined use in silencing
>> -Wuninitialized warnings.
>
> IMO, that's a myth. The construct invokes undefined behavior at least
> since C99, and the compilers are right to complain about it.
And I complained about this a couple months ago, as the compiler on
HP-NonStop stumbles across this too (by emitting a warning)
Bye, Jojo
^ permalink raw reply
* [PATCH] Documentation: add a README file
From: Yann Droneaud @ 2013-03-21 13:45 UTC (permalink / raw)
To: Git; +Cc: Yann Droneaud
The documentation is in AsciiDoc format: it should be written somewhere
with links to AsciiDoc documentation so that it became easy to find
how to write documentation for Git.
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
Documentation/README | 13 +++++++++++++
1 file changed, 13 insertions(+)
create mode 100644 Documentation/README
diff --git a/Documentation/README b/Documentation/README
new file mode 100644
index 0000000..c41734c
--- /dev/null
+++ b/Documentation/README
@@ -0,0 +1,13 @@
+Documentation
+=============
+
+Most of the Git documentation is in AsciiDoc format,
+a lightweight markup text language.
+
+AsciiDoc formatted files can be translated to man pages,
+HTML, DocBook or PDF documents.
+
+See:
+- http://asciidoc.org/
+- https://git.wiki.kernel.org/index.php/AsciiDoc
+
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings
From: Joachim Schmitz @ 2013-03-21 13:56 UTC (permalink / raw)
To: git
In-Reply-To: <kif2so$2ug$1@ger.gmane.org>
Joachim Schmitz wrote:
> Johannes Sixt wrote:
>> Am 3/21/2013 12:03, schrieb Jeff King:
>>> I was fooling around with clang and noticed that it complains about
>>> the "int x = x" construct under -Wall. That is IMHO a deficiency in
>>> clang, since the idiom has a well-defined use in silencing
>>> -Wuninitialized warnings.
>>
>> IMO, that's a myth. The construct invokes undefined behavior at least
>> since C99, and the compilers are right to complain about it.
>
> And I complained about this a couple months ago, as the compiler on
Actually on August 20th, 2012...
> HP-NonStop stumbles across this too (by emitting a warning)
Bye, Jojo
^ permalink raw reply
* Re: [RFC] Add posibility to preload stat information.
From: Junio C Hamano @ 2013-03-21 14:44 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jeff King, Fredrik Gustafsson, spearce, git, pclouds
In-Reply-To: <87vc8lyqz7.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> I think it would actually be a somewhat interesting feature if it
> interacted with GIT_PS1_SHOW*. If you use these settings (I personally
> use SHOWDIRTYSTATE but not SHOWUNTRACKEDFILES), the prompt hangs while
> __git_ps1 runs git-status. It should be possible to run a git-status
> process in the background when entering a repository, and displaying
> some marker ('??' maybe) in the prompt instead of the dirty-state info
> until git-status has finished.
This is somewhat interesting.
Perhaps we can introduce a helper binary that does what __git_ps1()
does, with a --timeout=500ms option to say "I dunno (yet)", and keep
priming the well in the background when it takes more than the
specified amount of time?
^ permalink raw reply
* Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings
From: Junio C Hamano @ 2013-03-21 14:58 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git
In-Reply-To: <20130321115545.GB21319@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Git code was my introduction to it, too, and I was led to believe it was
> idiomatic, so I can't speak further on that. I think it was Junio who
> introduced me to it, so maybe he can shed more light on the history.
I think we picked the convention up from the kernel folks. At least
that is how I first met the construct. The uninitialized_var(x)
macro was (and still is) used to mark these "The compiler is too
dumb to realize, but we know what we are doing" cases:
$ git grep '#define uninitialized_var' include/
include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
include/linux/compiler-intel.h:#define uninitialized_var(x) x
but they recently had a discussion, e.g.
http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705
so...
^ permalink raw reply
* Re: [PATCH] do not use GIT_TRACE_PACKET=3 in tests
From: Junio C Hamano @ 2013-03-21 15:04 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Git Mailing List
In-Reply-To: <514AAA5D.4000803@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Am 3/20/2013 18:43, schrieb Jeff King:
>> Now that we are using the GIT_TRACE mechanism, we can
>> improve both scripts by asking git to write directly to a
>> file rather than a descriptor. That fixes the hang in t5700,
>> and should allow t5503 to successfully run on Windows.
>
> Well spotted, and, right, both tests pass with this patch.
>
> Tested-by: Johannes Sixt <j6t@kdbg.org>
Thanks.
^ permalink raw reply
* Re: [PATCH 3/4] drop some obsolete "x = x" compiler warning hacks
From: Erik Faye-Lund @ 2013-03-21 15:16 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130321111028.GC18819@sigill.intra.peff.net>
On Thu, Mar 21, 2013 at 12:10 PM, Jeff King <peff@peff.net> wrote:
> In cases where the setting and access of a variable are
> protected by the same conditional flag, older versions of
> gcc would generate a "might be used unitialized" warning. We
> silence the warning by initializing the variable to itself,
> a hack that gcc recognizes.
>
> Modern versions of gcc are smart enough to get this right,
> going back to at least version 4.3.5. gcc 4.1 does get it
> wrong in both cases, but is sufficiently old that we
> probably don't need to care about it anymore.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> gcc 4.2 is conspicuously missing because no current Debian system even
> has a backwards-compatibility package for it, making it harder to test.
> And 4.3 was old enough for me to say "I do not care if you can run with
> -Wall -Werror or not", let alone 4.2.
Just a data-point. This is the version we use in msysGit:
$ gcc --version
gcc.exe (TDM-1 mingw32) 4.4.0
So yeah, it's not going to increase false positives here, I guess.
^ permalink raw reply
* Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings
From: Junio C Hamano @ 2013-03-21 15:19 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git
In-Reply-To: <7vppysbxzo.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> Git code was my introduction to it, too, and I was led to believe it was
>> idiomatic, so I can't speak further on that. I think it was Junio who
>> introduced me to it, so maybe he can shed more light on the history.
>
> I think we picked the convention up from the kernel folks. At least
> that is how I first met the construct. The uninitialized_var(x)
> macro was (and still is) used to mark these "The compiler is too
> dumb to realize, but we know what we are doing" cases:
>
> $ git grep '#define uninitialized_var' include/
> include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
> include/linux/compiler-intel.h:#define uninitialized_var(x) x
>
> but they recently had a discussion, e.g.
>
> http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705
>
> so...
While flipping the paragraphs around before sending the message out
I managed to lose the important one. Here is roughly what I wrote:
I am for dropping "= x" and leaving it uninitialized at the
declaration site, or explicitly initializing it to some
reasonable starting value (e.g. NULL if it is a pointer) and
adding a comment to say that the initialization is to squelch
compiler warnings.
^ permalink raw reply
* Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
From: Junio C Hamano @ 2013-03-21 15:29 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1363869587-10462-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index be809e5..e974b11 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
> ($sender) = expand_aliases($sender) if defined $sender;
>
> # returns 1 if the conflict must be solved using it as a format-patch argument
> -sub check_file_rev_conflict($) {
> +sub check_file_rev_conflict {
Have you verified that the callers of this sub are OK with this
change? It used to force scalar context on its arguments but now it
does not.
I am not saying I know the callers will get broken. I am trying to
make sure that this is *not* the result of blindly following
perlcritic output without understanding the ramifications of the
change.
> @@ -1438,7 +1438,7 @@ sub recipients_cmd {
>
> my $sanitized_sender = sanitize_address($sender);
> my @addresses = ();
> - open my $fh, "$cmd \Q$file\E |"
> + open my $fh, q{-|}, "$cmd \Q$file\E"
Strange quoting (why not just say "-|"?) aside, if you are moving
away from the two-param form of open(), it would be a sane thing to
do to also stop concatenating the arguments to commands to avoid
shell metacharacter gotcha. It will make the resulting code much
safer.
^ permalink raw reply
* Re: [PATCH] Documentation: add a README file
From: Junio C Hamano @ 2013-03-21 15:31 UTC (permalink / raw)
To: Yann Droneaud; +Cc: Git
In-Reply-To: <1363873555-8274-1-git-send-email-ydroneaud@opteya.com>
Yann Droneaud <ydroneaud@opteya.com> writes:
> The documentation is in AsciiDoc format: it should be written somewhere
> with links to AsciiDoc documentation so that it became easy to find
> how to write documentation for Git.
Certainly this does not deserve a *new* file to hold it.
Isn't this inferrable from the top-level INSTALL already?
^ permalink raw reply
* Re: [PATCH 0/4] drop some "int x = x" hacks to silence gcc warnings
From: Jeff King @ 2013-03-21 15:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <7vhak4bx0w.fsf@alter.siamese.dyndns.org>
On Thu, Mar 21, 2013 at 08:19:43AM -0700, Junio C Hamano wrote:
> > $ git grep '#define uninitialized_var' include/
> > include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
> > include/linux/compiler-intel.h:#define uninitialized_var(x) x
> >
> > but they recently had a discussion, e.g.
> >
> > http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705
> >
> > so...
>
> While flipping the paragraphs around before sending the message out
> I managed to lose the important one. Here is roughly what I wrote:
>
> I am for dropping "= x" and leaving it uninitialized at the
> declaration site, or explicitly initializing it to some
> reasonable starting value (e.g. NULL if it is a pointer) and
> adding a comment to say that the initialization is to squelch
> compiler warnings.
I'd be in favor of that, too. In many cases, I think the fact that gcc
cannot trace the control flow is a good indication that it is hard for a
human to trace it, too. And in those cases we would be better off
restructuring the code slightly to make it more obvious to both types of
readers.
Two patches to follow.
[5/4]: fast-import: clarify "inline" logic in file_change_m
[6/4]: run-command: always set failed_errno in start_command
-Peff
^ permalink raw reply
* [PATCH 5/4] fast-import: clarify "inline" logic in file_change_m
From: Jeff King @ 2013-03-21 15:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <20130321154402.GA25907@sigill.intra.peff.net>
When we read a fast-import line like:
M 100644 :1 foo.c
we point the local object_entry variable "oe" to the object
named by the mark ":1". When the input uses the "inline"
construct, however, we do not have such an object_entry.
The current code is careful not to access "oe" in the inline
case, but we can make the assumption even more obvious (and
catch violations of it) by setting oe to NULL and adding a
comment. As a bonus, this also squelches an over-zealous gcc
-Wuninitialized warning, which means we can drop the "oe =
oe" initialization hack.
Signed-off-by: Jeff King <peff@peff.net>
---
fast-import.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fast-import.c b/fast-import.c
index e12a8b8..a0c2c2f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2265,7 +2265,7 @@ static void file_change_m(struct branch *b)
const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
- struct object_entry *oe = oe;
+ struct object_entry *oe;
unsigned char sha1[20];
uint16_t mode, inline_data = 0;
@@ -2292,6 +2292,7 @@ static void file_change_m(struct branch *b)
hashcpy(sha1, oe->idx.sha1);
} else if (!prefixcmp(p, "inline ")) {
inline_data = 1;
+ oe = NULL; /* not used with inline_data, but makes gcc happy */
p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
--
1.8.2.rc2.8.g2161951
^ permalink raw reply related
* [PATCH 6/4] run-command: always set failed_errno in start_command
From: Jeff King @ 2013-03-21 15:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <20130321154402.GA25907@sigill.intra.peff.net>
When we fail to fork, we set the failed_errno variable to
the value of errno so it is not clobbered by later syscalls.
However, we do so in a conditional, and it is hard to see
later under what conditions the variable has a valid value.
Instead of setting it only when fork fails, let's just
always set it after forking. This is more obvious for human
readers (as we are no longer setting it as a side effect of
a strerror call), and it is more obvious to gcc, which no
longer generates a spurious -Wuninitialized warning. It also
happens to match what the WIN32 half of the #ifdef does.
Signed-off-by: Jeff King <peff@peff.net>
---
run-command.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index 07e27ff..765c2ce 100644
--- a/run-command.c
+++ b/run-command.c
@@ -273,7 +273,7 @@ int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
int fdin[2], fdout[2], fderr[2];
- int failed_errno = failed_errno;
+ int failed_errno;
char *str;
/*
@@ -341,6 +341,7 @@ fail_pipe:
notify_pipe[0] = notify_pipe[1] = -1;
cmd->pid = fork();
+ failed_errno = errno;
if (!cmd->pid) {
/*
* Redirect the channel to write syscall error messages to
@@ -420,7 +421,7 @@ fail_pipe:
}
if (cmd->pid < 0)
error("cannot fork() for %s: %s", cmd->argv[0],
- strerror(failed_errno = errno));
+ strerror(errno));
else if (cmd->clean_on_exit)
mark_child_for_cleanup(cmd->pid);
--
1.8.2.rc2.8.g2161951
^ permalink raw reply related
* Re: Git on Mac OS and precomposed unicode
From: Torsten Bögershausen @ 2013-03-21 16:04 UTC (permalink / raw)
To: Leo Koppelkamm; +Cc: git, Torsten Bögershausen
In-Reply-To: <64B4EB934AFE4B358B94EB717103887E@leo-koppelkamm.de>
On 21.03.13 14:00, Leo Koppelkamm wrote:
> Torsten Bögershausen added a patch for this a while ago.
> It seems it only works for files, not for folders with unicode in it.
>
> Eg. on ubuntu box:
> git init
> mkdir hä
> touch hä/file
> git add hä
> git commit
>
> Later on Mac
> git clone ………
> git status
>
> # On branch master
> # Untracked files:
> # (use "git add <file>..." to include in what will be committed)
> #
> # hä/
>
> Regards Leo
That is what I read from the commit message:
When creating a new git repository with "git init" or "git clone",
"core.precomposedunicode" will be set "false".
The user needs to activate this feature manually. She typically
sets core.precomposedunicode to "true" on HFS and VFAT, or file
systems mounted via SAMBA.
(Which is slighty wrong, because the name of the config variable
is core.precomposeunicode)
What does
git config core.precomposeunicode
say ?
You may consider to set it to set it to true globally,
git config --global core.precomposeunicode true,
and the next "git clone" will work as expected.
On the other hand,
could/should we consider to change the default to true in Git 2.0?
HTH
/Torsten
What does git config
^ permalink raw reply
* Re: [PATCH] Documentation: add a README file
From: Yann Droneaud @ 2013-03-21 15:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7v8v5gbwh6.fsf@alter.siamese.dyndns.org>
Le 21.03.2013 16:31, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@opteya.com> writes:
>
>> The documentation is in AsciiDoc format: it should be written
>> somewhere
>> with links to AsciiDoc documentation so that it became easy to find
>> how to write documentation for Git.
>
> Certainly this does not deserve a *new* file to hold it.
That's the first one I look for.
There were no indication about how to write documentation
in SubmittingPatches.
Later, I've found the only useful piece of advice regarding the
documentation
in howto/new-command.txt:
"
What every extension command needs
----------------------------------
You must have a man page, written in asciidoc (this is what Git help
followed by your subcommand name will display). Be aware that there is
a local asciidoc configuration and macros which you should use. It's
often helpful to start by cloning an existing page and replacing the
text content.
"
And I was grep'ing for ascii[ _-]doc throughout the sources !
> Isn't this inferrable from the top-level INSTALL already?
In short: No.
You want me to look in the INSTALL file to search for the syntax
to write documentation ?
For someone who don't follow Git development, it's really not the file
you're looking for.
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: Git on Mac OS and precomposed unicode
From: Leo Koppelkamm @ 2013-03-21 16:30 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <514B2F98.30409@web.de>
I found the problem:
I copied the config out of your commit message, where you wrote core.precompose****d****unicode
Turns out the d is too much. It seems to work now.
Leo
On Thursday, 21 March 2013 at 17:04, Torsten Bögershausen wrote:
> On 21.03.13 14:00, Leo Koppelkamm wrote:
> > Torsten Bögershausen added a patch for this a while ago.
> > It seems it only works for files, not for folders with unicode in it.
> >
> > Eg. on ubuntu box:
> > git init
> > mkdir hä
> > touch hä/file
> > git add hä
> > git commit
> >
> > Later on Mac
> > git clone ………
> > git status
> >
> > # On branch master
> > # Untracked files:
> > # (use "git add <file>..." to include in what will be committed)
> > #
> > # hä/
> >
> > Regards Leo
> That is what I read from the commit message:
>
> When creating a new git repository with "git init" or "git clone",
> "core.precomposedunicode" will be set "false".
>
> The user needs to activate this feature manually. She typically
> sets core.precomposedunicode to "true" on HFS and VFAT, or file
> systems mounted via SAMBA.
>
> (Which is slighty wrong, because the name of the config variable
> is core.precomposeunicode)
>
> What does
> git config core.precomposeunicode
> say ?
>
> You may consider to set it to set it to true globally,
> git config --global core.precomposeunicode true,
> and the next "git clone" will work as expected.
>
> On the other hand,
> could/should we consider to change the default to true in Git 2.0?
>
> HTH
> /Torsten
>
>
>
>
>
>
>
>
>
> What does git config
^ permalink raw reply
* Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
From: Ramkumar Ramachandra @ 2013-03-21 16:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7vd2usbwkn.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index be809e5..e974b11 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
>> ($sender) = expand_aliases($sender) if defined $sender;
>>
>> # returns 1 if the conflict must be solved using it as a format-patch argument
>> -sub check_file_rev_conflict($) {
>> +sub check_file_rev_conflict {
>
> Have you verified that the callers of this sub are OK with this
> change? It used to force scalar context on its arguments but now it
> does not.
>
> I am not saying I know the callers will get broken. I am trying to
> make sure that this is *not* the result of blindly following
> perlcritic output without understanding the ramifications of the
> change.
No Junio, I haven't blindly followed the perlcritic output. I've
considered the ramifications, audited the callers, and run the tests
to make sure that nothing breaks.
>> @@ -1438,7 +1438,7 @@ sub recipients_cmd {
>>
>> my $sanitized_sender = sanitize_address($sender);
>> my @addresses = ();
>> - open my $fh, "$cmd \Q$file\E |"
>> + open my $fh, q{-|}, "$cmd \Q$file\E"
>
> Strange quoting (why not just say "-|"?) aside
Intentional. I thought it was a pretty way to differentiate between a
mode string (which can only be <, >, or -|) and a filename string. If
you don't share my taste, feel free to use quotes instead.
> , if you are moving
> away from the two-param form of open(), it would be a sane thing to
> do to also stop concatenating the arguments to commands to avoid
> shell metacharacter gotcha. It will make the resulting code much
> safer.
Yes, the file can be improved in many ways, but that is not the topic
of this series. This series just makes the changes suggested by
perlcritic gentle.
^ permalink raw reply
* Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
From: Junio C Hamano @ 2013-03-21 17:28 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <20130321054349.GA18101@duynguyen-vnpc.dek-tpc.internal>
Duy Nguyen <pclouds@gmail.com> writes:
>> I still can't reproduce it. But I think I found a bug that
>> miscalculates prefix length from absolute paths. Does this "fix" your
>> test?
>> ...
> Nope, that one could cause more crashes. Try this
>
> -- 8< --
> diff --git a/setup.c b/setup.c
> index 3584f22..3d8eb97 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -14,6 +14,8 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
> const char *temp = real_path(path);
> sanitized = xmalloc(len + strlen(temp) + 1);
> strcpy(sanitized, temp);
> + if (p_len)
> + *p_len = 0;
Yes, this one seems to. "$(pwd)/../src" was not handled correctly.
The callchain to this locaiton would look like
parse_pathspec() with prefix="docs/", prefixlen set to 5
-> prefix_pathspec(), &prefixlen passed down
-> prefix_path_gently(), p_len points at the above prefixlen
your "this should fix" patch sets *p_len to 0,
original leaves *p_len as 5.
-> normalize_path_copy_len() with p_len
*p_len is used here.
Why could the test pass for you without it? It doesn't look like a
bug that depended on uninitialized memory or something from the
above observation.
^ permalink raw reply
* Re: [PATCH] Documentation: add a README file
From: Junio C Hamano @ 2013-03-21 17:39 UTC (permalink / raw)
To: Yann Droneaud; +Cc: Git
In-Reply-To: <64e67681cf5584b51bc84082fe6304c0@meuh.org>
Yann Droneaud <ydroneaud@opteya.com> writes:
> There were no indication about how to write documentation
> in SubmittingPatches.
I would agree that is probably the right place for it if we were to
add insns/hints.
^ permalink raw reply
* Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
From: Junio C Hamano @ 2013-03-21 17:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <7v4ng4br1y.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> I still can't reproduce it. But I think I found a bug that
>>> miscalculates prefix length from absolute paths. Does this "fix" your
>>> test?
>>> ...
>> Nope, that one could cause more crashes. Try this
>>
>> -- 8< --
>> diff --git a/setup.c b/setup.c
>> index 3584f22..3d8eb97 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -14,6 +14,8 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
>> const char *temp = real_path(path);
>> sanitized = xmalloc(len + strlen(temp) + 1);
>> strcpy(sanitized, temp);
>> + if (p_len)
>> + *p_len = 0;
>
> Yes, this one seems to. "$(pwd)/../src" was not handled correctly.
>
> The callchain to this locaiton would look like
>
> parse_pathspec() with prefix="docs/", prefixlen set to 5
> -> prefix_pathspec(), &prefixlen passed down
> -> prefix_path_gently(), p_len points at the above prefixlen
> your "this should fix" patch sets *p_len to 0,
> original leaves *p_len as 5.
> -> normalize_path_copy_len() with p_len
> *p_len is used here.
>
> Why could the test pass for you without it? It doesn't look like a
> bug that depended on uninitialized memory or something from the
> above observation.
The change made to prefix_path_gently() in this series is beyond
"disgusting", especially with the above fix-up.
Sometimes it uses the original "len", sometimes it uses the fixed-up
*p_len (e.g. passes it down to normalize_path_copy_len()), and lets
normalize_path_copy_len() further update it, and thenit makes the
caller use the updated *p_len.
Does the caller know what the value in *p_len _mean_ after this
function returns? Can it afford to lose the original length of the
prefix it saved in a variable, without getting confused?
I think any change that turns a value-passed argument in the
existing code into modifiable pointer-to-variable in this series
should add in-code comment to describe what the variable mean upon
entry and after return, just like normalize_path_copy_len() that was
built out of the original normalize_path_copy(). I didn't look if
there are many others, or if this is the only one that is tricky. it
is tricky that even the original author of the patch got it wrong
X-<.
^ permalink raw reply
* Re: [PATCH v9 3/5] Implement line-history search (git log -L)
From: Junio C Hamano @ 2013-03-21 19:05 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <b8cabd5ca63a17577fca524891046e5a3d3dfc60.1363865444.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> +void line_log_init(struct rev_info *rev, const char *prefix, struct string_list *args)
> +{
> + struct commit *commit = NULL;
> + struct line_log_data *range;
> +
> + commit = check_single_commit(rev);
> + range = parse_lines(commit, prefix, args);
> + add_line_range(rev, commit, range);
> +
> + if (!rev->diffopt.detect_rename) {
> + int i, count = 0;
> + struct line_log_data *r = range;
> + const char **paths;
> + while (r) {
> + count++;
> + r = r->next;
> + }
> + paths = xmalloc((count+1)*sizeof(char *));
> + r = range;
> + for (i = 0; i < count; i++) {
> + paths[i] = xstrdup(r->spec->path);
> + r = r->next;
> + }
> + paths[count] = NULL;
> + init_pathspec(&rev->diffopt.pathspec, paths);
> + free(paths);
> + }
> +}
Why not do the pathspec limitation under "-M"?
It is not like you are picking up origins of blocks of lines copied
or moved from other files like "blame -C" does, so I suspect it
would be simpler to mimic what --follow does, which is to (1) use
the pathspec to follow the paths you care about, and then (2) when
you find one or more of the paths you were following disappear in a
commit, only at that point you re-check to see if there are other
paths that existed in the parent that the disappeared paths were
renamed from, and match with that one.
^ permalink raw reply
* Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable
From: Jonathan Nieder @ 2013-03-21 19:49 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130321110527.GA18819@sigill.intra.peff.net>
Jeff King wrote:
> Instead of using the "x = x" hack, let's handle the default
> case in the switch() statement with a die("BUG"). That tells
> the compiler and any readers of the code exactly what the
> function's input assumptions are.
Sounds reasonable.
> We could also convert the flag to an enum, which would
> provide a compile-time check on the function input.
Unfortunately C permits out-of-bounds values for enums.
[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,
> {
> struct wt_status_change_data *d = it->util;
> const char *c = color(change_type, s);
> - int status = status;
> + int status;
> char *one_name;
> char *two_name;
> const char *one, *two;
> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,
> }
> status = d->worktree_status;
> break;
> + default:
> + die("BUG: unhandled change_type %d in wt_status_print_change_data",
> + change_type);
Micronit: s/unhandled/invalid/.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] Documentation: merging a tag is a special case
From: Junio C Hamano @ 2013-03-21 19:50 UTC (permalink / raw)
To: Yann Droneaud; +Cc: Git, Jonathan Nieder
In-Reply-To: <7vboadevpk.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> +MERGING TAG
>> +-----------
>> +
>> +When merging a tag (annotated or signed), Git will create a merge commit
>> +...
>> +if the tag was signed. See also linkgit:git-tag[1].
>> +
>
> It would make it more helpful to readers to describe how _not_ to
> create such a merge commit if it is unwanted, and how the request to
> merge a tag interacts with --ff-only option.
>
>> @@ -26,7 +26,7 @@ set to `no` at the beginning of them.
>> --ff::
>> When the merge resolves as a fast-forward, only update the branch
>> pointer, without creating a merge commit. This is the default
>> - behavior.
>> + behavior (except when merging a tag).
>
> With this update, the reader will be left wondering what would be
> the default when she asks Git to merge a tag, no?
Taking Jonathan's input and the above into account, perhaps we can
do something like this on top of the posted patch?
Documentation/git-merge.txt | 26 +++++++++++++++++++++-----
Documentation/merge-options.txt | 5 +++--
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index f7e68e1..75b5ee7 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -173,11 +173,27 @@ want to start over, you can recover with `git merge --abort`.
MERGING TAG
-----------
-When merging a tag (annotated or signed), Git will create a merge commit
-even if a fast-forward merge is possible (see above).
-The commit message template will be created from the tag message.
-Additionally, the signature check will be reported as a comment
-if the tag was signed. See also linkgit:git-tag[1].
+When merging an annotated (and possibly signed) tag, Git always
+creates a merge commit even if a fast-forward merge is possible, and
+the commit message template is prepared with the tag message.
+Additionally, the signature check is reported as a comment
+if the tag is signed. See also linkgit:git-tag[1].
+
+Consequently a request `git merge --ff-only v1.2.3` to merge such a
+tag would fail.
+
+When you want to just integrate with the work leading to the commit
+that happens to be tagged, e.g. synchronizing with an upstream
+release point, you may not want to make an unnecessary merge commit
+especially when you do not have any work on your own. In such a
+case, you can "unwrap" the tag yourself before feeding it to `git
+merge`, e.g.
+
+---
+git fetch origin
+git merge [--ff-only] v1.2.3^0
+---
+
HOW CONFLICTS ARE PRESENTED
---------------------------
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 70d1ec0..34a8445 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -26,11 +26,12 @@ set to `no` at the beginning of them.
--ff::
When the merge resolves as a fast-forward, only update the branch
pointer, without creating a merge commit. This is the default
- behavior (except when merging a tag).
+ behavior.
--no-ff::
Create a merge commit even when the merge resolves as a
- fast-forward.
+ fast-forward. This is the default behaviour when merging an
+ annotated (and possibly signed) tag.
--ff-only::
Refuse to merge and exit with a non-zero status unless the
^ permalink raw reply related
* [PATCH] diff.c: diff.renamelimit => diff.renameLimit in message
From: Max Nanasy @ 2013-03-21 19:53 UTC (permalink / raw)
To: git
In the warning message printed when rename or unmodified copy
detection was skipped due to too many files, change "diff.renamelimit"
to "diff.renameLimit", in order to make it consistent with git
documentation, which consistently uses "diff.renameLimit".
Signed-off-by: Max Nanasy <max.nanasy@gmail.com>
---
diff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index 156fec4..052974e 100644
--- a/diff.c
+++ b/diff.c
@@ -4662,7 +4662,7 @@ int diff_result_code(struct diff_options *opt, int status)
{
int result = 0;
- diff_warn_rename_limit("diff.renamelimit",
+ diff_warn_rename_limit("diff.renameLimit",
opt->needed_rename_limit,
opt->degraded_cc_to_c);
if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
--
1.8.1.3
^ permalink raw reply related
* Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable
From: Junio C Hamano @ 2013-03-21 19:55 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git
In-Reply-To: <20130321194949.GG29311@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Jeff King wrote:
>
>> Instead of using the "x = x" hack, let's handle the default
>> case in the switch() statement with a die("BUG"). That tells
>> the compiler and any readers of the code exactly what the
>> function's input assumptions are.
>
> Sounds reasonable.
>
>> We could also convert the flag to an enum, which would
>> provide a compile-time check on the function input.
>
> Unfortunately C permits out-of-bounds values for enums.
>
> [...]
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,
>> {
>> struct wt_status_change_data *d = it->util;
>> const char *c = color(change_type, s);
>> - int status = status;
>> + int status;
>> char *one_name;
>> char *two_name;
>> const char *one, *two;
>> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,
>> }
>> status = d->worktree_status;
>> break;
>> + default:
>> + die("BUG: unhandled change_type %d in wt_status_print_change_data",
>> + change_type);
>
> Micronit: s/unhandled/invalid/.
I actually think "unhandled" is more correct for this one; we may
add new change_type later in the caller, and we do not want to
forget to add a new case arm that handles the new value.
^ 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