* Re: [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls
From: Erik Faye-Lund @ 2011-12-15 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, schwab
In-Reply-To: <20111214181658.GA1691@sigill.intra.peff.net>
On Wed, Dec 14, 2011 at 7:16 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote:
>
>> This avoids us from accidentally dropping state, possibly leading
>> to unexpected behaviour.
>
> I do think this is fine in a "be extra cautious" kind of way.
>
>> This is especially important on Windows, where the maximum size of
>> the environment is 32 kB.
>
> But does your patch actually detect that? As Andreas pointed out, these
> limits don't typically come into play at setenv time. Instead, the
> environment is allocated on the heap, and then the result is passed to
> exec/spawn, which will fail.
>
> So your patch is really detecting a failure to malloc, not an overflow
> of the environment size, and Windows is just as (un)likely to run out of
> heap as any other platform.
>
You are right; I just assumed that our setenv was implemented on top
of SetEnvironmentVariable, which does impose max-limits on setting (if
I read the documentation correctly):
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206(v=vs.85).aspx
But we don't. We have mingw_setenv, which simply modifies environ.
> You can check how your platform behaves by applying this patch:
>
> diff --git a/git.c b/git.c
> index f10e434..57f6b12 100644
> --- a/git.c
> +++ b/git.c
> @@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv)
> alias_argv[i] = (*argv)[i];
> alias_argv[argc] = NULL;
>
> + /* make gigantic environment */
> + {
> + int len = 256 * 1024;
> + char *buf = xmalloc(len);
> + memset(buf, 'z', len);
> + buf[len-1] = '\0';
> + if (setenv("FOO", buf, 1))
> + die("setenv failed");
> + }
> +
> ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
> if (ret >= 0) /* normal exit */
> exit(ret);
>
> and then running:
>
> git -c alias.foo='!echo ok' foo
>
> which yields:
>
> fatal: cannot exec 'echo ok': Argument list too long
>
> on Linux.
>
Yeah, but I'm getting:
$ git -c alias.foo='!echo ok' foo
ok
Which is strange to me.
But if I do:
$ git -c alias.foo='!echo $FOO' foo
it does echo a bunch of 'z's. And we get the expected amount:
$ git -c alias.foo='!echo $FOO' foo | wc -c
262144
This strikes me as very strange, because we end up calling the ANSI
version of CreateProcess with an environment-block beyond 32767 chars,
which MSDN says should fail:
"The ANSI version of this function, CreateProcessA fails if the total
size of the environment block for the process exceeds 32,767
characters."
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
Oh well, this is strange behavior in a good way; I'm not going to complain :P
^ permalink raw reply
* Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))
From: Junio C Hamano @ 2011-12-15 18:19 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, Thomas Rast, git,
Nguyễn Thái Ngọc Duy, Michael Haggerty
In-Reply-To: <20111215065529.GA1327@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > This correctly detects the bug in t7006. I can't decide if it's clever
>> > or ugly.
>>
>> I would say it falls on the latter side of the line by small margin. Let's
>> do the /dev/null thing and be done with it.
>
> Darn, I wanted to post it on my fridge with an "A+".
Heh.
If it were without the perl "tell" bits, it would have been "clever and
clean", perhaps like
arrange the following to read from t/random-stdin file
- run the test script
- "read x"
compare the $x you just read with t/random-stdin file
But I think it is good enough to be stupid and clean.
> Here's a cleaned-up version of the /dev/null one.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] push: --ignore-stale option
From: Junio C Hamano @ 2011-12-15 18:21 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111215073816.GD1327@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But I still see this as solving the wrong problem, and encouraging a
> dangerous workflow.
Yes, that is what I meant, so we are in total agreement that these people
should be encouraged not to use the matching semantics.
How that encouragement is made we may differ, though.
^ permalink raw reply
* Re: [PATCH v2 1/3] merge: abort if fails to commit
From: Junio C Hamano @ 2011-12-15 18:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Miles Bader
In-Reply-To: <1323956843-5326-1-git-send-email-pclouds@gmail.com>
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/4] wrapper: supply xsetenv and xputenv
From: Erik Faye-Lund @ 2011-12-15 18:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, schwab
In-Reply-To: <7vzketohvg.fsf@alter.siamese.dyndns.org>
On Thu, Dec 15, 2011 at 6:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> +extern int xsetenv(const char *name, const char *val, int override);
>> +extern int xputenv(const char *string);
>
> Actually, putenv(3) takes "char *string".
Ah, thanks for spotting.
> I adjusted 3 & 4 locally before queuing, so there is no need for resend,
> and judging from the later part of the discussion, it seems that it may be
> better to use only the first two patches from this series after all.
Yeah, I agree; patch 1 and 2 are the only ones that makes sense.
^ permalink raw reply
* Re: [PATCH 3/3] Do not create commits whose message contains NUL
From: Junio C Hamano @ 2011-12-15 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, Miles Bader
In-Reply-To: <20111214182953.GA6469@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm happy to ignore custom programs linking against internal git code,
> but what should "git commit-tree" do?
>
> My gut feeling is that it should store the literal binary contents.
> However, I don't think this has ever been the case. Even in the initial
> version of commit-tree.c, we read the input line-by-line and sprintf it
> into place.
Yeah, you are right. Perhaps we should tweak updated 3/3 to check at the
lower level commit_tree() then.
I've rewrote the log message for 2/3 as follows so we can go either way
;-)
Convert commit_tree() to take strbuf as message
There wan't a way for commit_tree() to notice if the message the caller
prepared contained a NUL byte, as it did not take the length of the
message as a parameter. Use a pointer to a strbuf instead, so that we can
either choose to allow low-level plumbing commands to make commits
that contain NUL byte in its message, or forbid NUL everywhere by
adding the check in commit_tree(), in later patches.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-lib: allow testing another git build tree
From: Junio C Hamano @ 2011-12-15 19:05 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Michael Haggerty
In-Reply-To: <3016269.uH93UWUbx5@thomas.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> On Wednesday 14 December 2011 19:07:35 Junio C Hamano wrote:
>> Thomas Rast <trast@student.ethz.ch> writes:
>> > The perf-lib work wants this feature, so we may as well do it for
>> > test-lib in general.
>>
>> How is this different from what GIT_TEST_INSTALLED already gives us
>> (other than "needs more diskspace to keep another source tree fully
>> built", that is)?
>
> I was scared away by the note that it would use (among others) perl
> libs from the current build tree. Upon investigation I also see that
> the test-* situation is still not satisfactory. Some (like
> test-chmtime) are used by the tests for a vital task, and if they ever
> have to be fixed, we would want to use the fixed version in any "test
> an old git" run. OTOH, others (e.g., test-dump-cache-tree) are linked
> with the rest of the code and serve to test an otherwise not
> accessible part of it, and testing an old git should use them from the
> tested tree.
Okay, so GIT_BUILD_DIR variant should now use everything from the tree
being tested for consistency to improve the situation, right?
Ehh, not quite, if you worry about an old one having broken test-chmtime,
you actively do not want that consistency; how is that addressed?
Not that I think we should address such issues---these standalone programs
tend to be small and simple, so always using them from the tree being
tested is a much better policy than using them from the tree you are
running the tests from.
> The disk space argument is moot IMO:
I was not really worried about "space" (disks are cheap) but more about
mental burden of having to keep multiple build trees in the source area.
Keeping multiple installed versions is already necessary to be able to
quickly respond to "I am using Git version X and this does not work".
^ permalink raw reply
* Re: [PATCH 3/3] Do not create commits whose message contains NUL
From: Junio C Hamano @ 2011-12-15 19:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy, Miles Bader
In-Reply-To: <7vehw5oepj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> My gut feeling is that it should store the literal binary contents.
>> However, I don't think this has ever been the case. Even in the initial
>> version of commit-tree.c, we read the input line-by-line and sprintf it
>> into place.
>
> Yeah, you are right. Perhaps we should tweak updated 3/3 to check at the
> lower level commit_tree() then.
>
> I've rewrote the log message for 2/3 as follows so we can go either way
> ;-)
s/rewrote/rewritten/ obviously...
> Convert commit_tree() to take strbuf as message
>
> There wan't a way for commit_tree() to notice if the message the caller
> prepared contained a NUL byte, as it did not take the length of the
> message as a parameter. Use a pointer to a strbuf instead, so that we can
> either choose to allow low-level plumbing commands to make commits
> that contain NUL byte in its message, or forbid NUL everywhere by
> adding the check in commit_tree(), in later patches.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
And 3/3 looks like this:
commit_tree(): refuse commit messages that contain NULs
Current implementation sees NUL as terminator. If users give a message
with NUL byte in it (e.g. editor set to save as UTF-16), the new commit
message will have NULs. However following operations (displaying or
amending a commit for example) will not keep anything after the first NUL.
Stop user right when they do this. If NUL is added by mistake, they have
their chance to fix. Otherwise, log messages will no longer be text "git
log" and friends would grok.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [msysGit] [PATCH] gitk: fix the display of files when filtered by path
From: Martin von Zweigbergk @ 2011-12-15 19:42 UTC (permalink / raw)
To: Pat Thoyts, Johannes Schindelin, Paul Mackerras, Junio C Hamano
Cc: David Aguilar, Git, msysGit
In-Reply-To: <alpine.DEB.1.00.1112151023280.2615@bonsai2>
Hi,
On Thu, Dec 15, 2011 at 1:24 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 14 Dec 2011, David Aguilar wrote:
>
>> On Tue, Dec 13, 2011 at 8:50 AM, Pat Thoyts
>> <patthoyts@users.sourceforge.net> wrote:
>> > Launching 'gitk -- .' or 'gitk -- ..\t' restricts the display to files
>> > under the given directory but the file list is left empty. This is because
>> > the path_filter function fails to match the filenames which are relative
>> > to the working tree to the filter which is filessytem relative.
>> > This solves the problem by making both names fully qualified filesystem
>> > paths before performing the comparison.
How is this related to my patches from April? See
http://thread.gmane.org/gmane.comp.version-control.git/170853. It's
clearly not the same problem, but will the patches conflict? Will some
of mine be unnecessary?
> Thanks for reminding me that I did not yet apply and push. Did so now.
What do you mean by this? Push to where?
git://git.kernel.org/pub/scm/gitk/gitk.git is still down.
Paul and Junio, the patches I sent in April are still not in git.git,
are they? Can we use another repo until the kernel.org one is up? More
than eight months to get a patch (or eight) merged is way too long,
IMO.
Martin
^ permalink raw reply
* Re: [msysGit] [PATCH] gitk: fix the display of files when filtered by path
From: Junio C Hamano @ 2011-12-15 19:50 UTC (permalink / raw)
To: Martin von Zweigbergk, Paul Mackerras; +Cc: git
In-Reply-To: <CAOeW2eHD7Xutf+pHDyMOo=uZC9PSFZi+aMq1Rx80iTKPFApr8A@mail.gmail.com>
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> Paul and Junio, the patches I sent in April are still not in git.git,
> are they? Can we use another repo until the kernel.org one is up? More
> than eight months to get a patch (or eight) merged is way too long,
> IMO.
I tend to agree.
I have this slight suspicion that Paul would appreciate if somebody who
cares about gitk who is capable and willing steps forward and takes over
the maintainership of gitk, as he is busy in his other projects.
^ permalink raw reply
* Re: Git difftool / mergetool on directory tree
From: Neal Kreitzinger @ 2011-12-15 19:53 UTC (permalink / raw)
To: Daniele Segato; +Cc: Git Mailing List
In-Reply-To: <4EE8618E.7020902@gmail.com>
On 12/14/2011 2:42 AM, Daniele Segato wrote:
> many diff / merge tool around have the ability to compare a directory
> tree (meld is one, but there are many).
>
> Is there a way to start a difftool or a mergetool on a folder instead of
> the single file?
>
> It would be an handsome feature to git.
>
> I googled a little before popping out this question and I only found
> suggestion on how to open "many" file at once instead of opening them
> serially but that's not the same thing not as powerful as directory
> comparison.
>
(This works with kdiff3 as the mergetool/difftool on git 1.7.1 on linux):
git difftool <commit>:<dir>/ <commit-2>:<dir>/
e.g., git difftool mycommit:ourdir/ yourcommit:ourdir/
<commit> = pointer to a commit (ie: sha-1, branchname, tag, ref)
<dir> = path of dir relative to worktree
v/r,
neal
^ permalink raw reply
* [msysGit] Windows & executable bit for newly created files
From: Dirk Süsserott @ 2011-12-15 20:07 UTC (permalink / raw)
To: Git Mailing List
Hi,
I sometimes write new (shell/perl) scripts under Windows whithin
msysGit's bash. An "ls -l" shows them as executable:
$ ls -l
-rwxr-xr-x ... 11 Dec 15 20:33 test.sh
Ok, "ls" just *pretends* they have the bit set (I think it just peeks
for the she-bang line).
Adding the file and looking at the patch gives:
$ git add test.sh
$ git diff --cached
diff --git a/test.sh b/test.sh
new file mode 100644
index 0000000..13f4793
--- /dev/null
+++ b/test.sh
@@ -0,0 +1,2 @@
+#!/bin/sh
+
Is there a way to convince git that the new mode is 755 instead of 644,
even with core.filemode set to false? So that the mode is correct when I
checkout the file under Linux later on?
My impression is that when git once knows the file has 755, it remembers
that. I'd like to avoid those commits (made under Linux) with "chmod +x
for scripts newly created under Windows".
Sometimes I create a script under Windows, add and commit it, create a
patch (git-format-patch), edit the patch (644 -> 755), reset the branch,
and finally git-am the patch. It works but is not particularly convenient.
Is there sth. like
$ git add --filemode=755 <filepattern> ?
Cheers,
Dirk
^ permalink raw reply
* Re: Branch names with slashes
From: Neal Kreitzinger @ 2011-12-15 20:40 UTC (permalink / raw)
To: Leonardo Kim; +Cc: git
In-Reply-To: <CAGcUY13TOodu1BO3DCoNnVvNZ9QkWAbD-RmyqQX6P1q6tcO+yg@mail.gmail.com>
On 12/14/2011 4:17 AM, Leonardo Kim wrote:
> Branch names can contain slashes, so we can use 'development/foo' as
> a branch name. If I choose 'development' as a branch name, it doesn't
> work. There is a directory named development at '.git/refs/heads'
> directory. So we cannot create a file named development for
> 'refs/heads/development'.
>
> An error message may occurs like below. Unfortunately, It is not of
> help to me. 'error: 'refs/heads/development/foo' exists; cannot
> create 'refs/heads/development'.
>
> I think that dealing with a file system and an error message above is
> not sufficient for a novice like me. I hope that it should be
> improved.
>
FYI, We also use slashes in our branchnames to leverage some of the
benefits of a path-like namespace like pattern matching and the logical
expression of hierarchies using descriptive compound names. (We use git
1.7.1 on linux.) Here's something to keep in mind: You have to plan
(think ahead) for your naming conventions so that the namespaces will
maintain uniqueness at a peer level over time that cannot be confused as
subdirs under .git/refs/heads. For example:
branchnames:
/mysystem/generic
/mysystem/generic/project1
will not work because /mysystem/generic appears to be a parent dir to
/mysystem/generic/project1 under .git/refs/heads. The solution is:
/mysystem/generic/base
/mysystem/generic/project1
these branches can coexist because they are unique without one appearing
to be a parent dir of the other. IOW, their namespaces are peers in
their entirety. To carry the example a little further,
/mysystem/generic/project1/part2
will not work because once again it appears to be a subdir of an
existing branchname (ref). However,
/mysystem/generic/project1-part2
will work.
I think the reason for this is that if you look at .git/refs/heads you
will see that these slash delimited branch names are actually stored as
subdirs in the filesystem sense. Therefore, git will get confused and
error out as it tries to find branchnames that are not entirely unique
by their full path namespace under .git/refs/heads because a branch
namespace that is a prefix (in the filesystem sense) of another branch
name would occupy the same path under .git/refs/heads without being
distinguishable as unique, and vice versa.
Hope this helps. (Maybe someone else will find a clearer way to explain
this.)
v/r,
neal
^ permalink raw reply
* Re: [PATCH 0/2] Improve git-bundle builtin
From: Jonathan Nieder @ 2011-12-15 20:54 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Joey Hess
In-Reply-To: <1323967528-10537-1-git-send-email-artagnon@gmail.com>
Hi Ram,
Ramkumar Ramachandra wrote:
> 1. There's a SP between the OBJID and the ref name in list-heads as
> opposed to the TAB used by other git commands such as ls-remote,
> diff-tree. Will fixing it break someone's parser somewhere?
I don't know. Would there be any advantage at all to changing the
output format of the tool? Bad idea.
If the goal is to avoid confusion, perhaps a note in the documentation
would help.
> 2. Is it worth fixing the "--stdin" tests? What is the usecase?
Is "script that wants to list which revs to bundle, possibly exceeding
the command-line length limit" not enough of a use case? Yes, I think
it is very much worth fixing.
Thanks for looking at this.
Jonathan
^ permalink raw reply
* [PATCH] Add '-P' as a synonym for '--no-pager' in the git command
From: Joe Ratterman @ 2011-12-15 20:55 UTC (permalink / raw)
To: gitster, git; +Cc: Joe Ratterman
Also, add both -P|--no-pager to the existing -p|--paginate in bash
completion.
Signed-off-by: Joe Ratterman <jratt0@gmail.com>
---
Documentation/git.txt | 3 ++-
contrib/completion/git-completion.bash | 2 +-
git.c | 4 ++--
| 8 ++++++++
4 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index e869032..c7f8445 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git' [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
- [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+ [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
[--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
[-c <name>=<value>]
[--help] <command> [<args>]
@@ -329,6 +329,7 @@ help ...`.
configuration options (see the "Configuration Mechanism" section
below).
+-P::
--no-pager::
Do not pipe git output into a pager.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index cc1bdf9..8b9ea1b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2640,7 +2640,7 @@ _git ()
case "$i" in
--git-dir=*) __git_dir="${i#--git-dir=}" ;;
--bare) __git_dir="." ;;
- --version|-p|--paginate) ;;
+ --version|-p|--paginate|-P|--no-pager) ;;
--help) command="help"; break ;;
*) command="$i"; break ;;
esac
diff --git a/git.c b/git.c
index 8e34903..baa1613 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
const char git_usage_string[] =
"git [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
- " [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]\n"
+ " [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n"
" [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
" [-c name=value] [--help]\n"
" <command> [<args>]";
@@ -103,7 +103,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
- } else if (!strcmp(cmd, "--no-pager")) {
+ } else if (!strcmp(cmd, "-P") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 320e1d1..783915e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -84,6 +84,14 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
! test -e paginated.out
'
+test_expect_success TTY 'no pager with -P' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
+ test_terminal git -P log &&
+ ! test -e paginated.out
+'
+
test_expect_success TTY 'no pager with --no-pager' '
rm -f paginated.out ||
cleanup_fail &&
--
1.7.7.1
^ permalink raw reply related
* Re: [msysGit] Windows & executable bit for newly created files
From: Junio C Hamano @ 2011-12-15 20:56 UTC (permalink / raw)
To: Dirk Süsserott; +Cc: Git Mailing List
In-Reply-To: <4EEA5387.5020808@dirk.my1.cc>
Dirk Süsserott <newsletter@dirk.my1.cc> writes:
> Is there a way to convince git that the new mode is 755 instead of 644,
> even with core.filemode set to false? So that the mode is correct when I
> checkout the file under Linux later on?
"git update-index --chmod=+x"?
^ permalink raw reply
* [PATCH] Gitweb: Avoid warnings when a repo does not have a valid HEAD
From: Joe Ratterman @ 2011-12-15 20:58 UTC (permalink / raw)
To: gitster, git, jnareb; +Cc: Joe Ratterman
It is possible that the HEAD reference does not point to an existing
branch. When viewing such a repository in gitweb, a message like this
one was sent to the error log:
gitweb.cgi: Use of uninitialized value in string eq at /usr/src/git/gitweb/gitweb.cgi line 5115.
Signed-off-by: Joe Ratterman <jratt0@gmail.com>
---
gitweb/gitweb.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f0c3bd..5af06d6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5440,7 +5440,7 @@ sub git_heads_body {
for (my $i = $from; $i <= $to; $i++) {
my $entry = $headlist->[$i];
my %ref = %$entry;
- my $curr = $ref{'id'} eq $head;
+ my $curr = $head ? ($ref{'id'} eq $head) : 0;
if ($alternate) {
print "<tr class=\"dark\">\n";
} else {
--
1.7.7.1
^ permalink raw reply related
* Re: [PATCH 1/2] t5704 (bundle): rewrite for larger coverage
From: Jonathan Nieder @ 2011-12-15 21:16 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Joey Hess
In-Reply-To: <1323967528-10537-2-git-send-email-artagnon@gmail.com>
Hi,
Ramkumar Ramachandra wrote:
> Rewrite
Always a scary word. Very rarely justified, especially when the
original and rewritten versions of something are not going to be able
to coexist for a period while the bugs are ironed out. It doesn't
leave me optimistic.
> the git-bundle testsuite to exercise more of its
> functionality.
Luckily, this goal suggests that I am going to see some new tests
added, without the existing coverage being removed or mangled, so
maybe I can ignore the fears awakened by the word "Rewrite". Let's
see...
[...]
> --- a/t/t5704-bundle.sh
> +++ b/t/t5704-bundle.sh
> @@ -1,56 +1,99 @@
> #!/bin/sh
>
> -test_description='some bundle related tests'
> +test_description='Test git-bundle'
Why?
> . ./test-lib.sh
>
> test_expect_success 'setup' '
> + git config core.logAllRefUpdates false &&
Why?
> + test_commit initial &&
> + git checkout -b branch &&
> + test_commit second &&
> + test_commit third &&
> + git checkout master &&
> + test_commit fourth file
> +'
No explicit tags in the setup this time. Now all commits are referred
to by tags, which worsens the coverage, since if some future change
caused commits not referred to by a tag to be dropped, it would be
missed. Paraphrasing
>file &&
git add file &&
test_tick &&
git commit -m initial &&
git tag -m initial initial
to
test_commit initial file
when not preparing to make some other change in the same place and if
the original was not too confusing feels like gratuitous churn.
[...]
> +test_expect_success 'create succeeds' '
> + git bundle create bundle second third &&
> + cat >expect <<-\EOF &&
> + OBJID refs/tags/third
> + OBJID refs/tags/second
> + EOF
> + {
> + git ls-remote bundle |
> + sed "s/$_x40/OBJID/g"
> + } >actual &&
> + test_cmp expect actual
> +'
A new test. What assertion is it testing? Why censor out the
object names when comparing the expected object names to the
actual ones, instead of computing the appropriate object names
for the expected result? Is this new test useful, or does it
cover ground already tested in t5510-fetch.sh?
> +test_expect_success 'verify succeeds' '
> + git bundle create bundle second third &&
> + git bundle verify bundle
> '
A test for "git bundle verify" is a welcome addition.
> +test_expect_success 'list-heads succeeds' '
> + git bundle create bundle second third &&
> + cat >expect <<-\EOF &&
> + OBJID refs/tags/second
> + OBJID refs/tags/third
> + EOF
> + {
> + git bundle list-heads bundle |
> + sed "s/$_x40/OBJID/g"
> + } >actual &&
> + test_cmp expect actual
> +'
> +
Based on 'git grep -e "git bundle list-heads" -- t', there don't seem
to be any existing tests for "git bundle list-heads" except for
t5510-fetch.sh, but I'm not sure what this adds on top of that one.
> -test_expect_success 'tags can be excluded by rev-list options' '
> -
> - git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
> - git ls-remote bundle > output &&
> - ! grep tag output
Dropped?
> +test_expect_success 'create dies on invalid bundle filename' '
> + mkdir adir &&
> + test_expect_code 128 git bundle create adir --all
> +'
How is "invalid bundle filename" a clearer explanation than "bundle
file cannot be created"?
>
> +test_expect_success 'disallow stray command-line options' '
> + test_must_fail git bundle create --junk bundle second third
> '
Ok. Might also be useful to check that no "--junk" or "bundle" file
is created.
> +test_expect_failure 'disallow stray command-line arguments' '
> + git bundle create bundle second third &&
> + test_must_fail git bundle verify bundle junk
> +'
In this case, "stray command-line arguments" actually means "extra
arguments to 'verify'", I guess?
What happens if I run "git bundle verify *.bundle" in a directory
with multiple bundles? What should happen?
> +test_expect_success 'create accepts rev-list options to narrow refs' '
> + git bundle create bundle --all -- file &&
I don't understand what "options to narrow refs" means. Does that
mean options like --remotes=origin which yield refs from some subset
of the ref namespace, unlike --all?
[...]
> +test_expect_success 'unbundle succeeds' '
A test for "git bundle unbundle" is a welcome addition.
[...]
> test_expect_failure 'bundle --stdin' '
> -
> echo master | git bundle create stdin-bundle.bdl --stdin &&
> git ls-remote stdin-bundle.bdl >output &&
> grep master output
> -
> '
Seems like a gratuitous change to mix into a patch that introduces
functional changes.
I found this hard to review, since it doesn't seem very focussed ---
it mixes style cleanups, removal of code, and introduction of new
code. I'd be way happier to see a new patch that just adds new tests
to the script without potentially breaking anything on the way. Then
if the style cleanups still seem important to you, they can be
reviewed as a separate patch.
Hoping that clarifies a little,
Jonathan
^ permalink raw reply
* Re: [PATCH] gitk: fix the display of files when filtered by path
From: Pat Thoyts @ 2011-12-15 21:18 UTC (permalink / raw)
To: Martin von Zweigbergk
Cc: msysGit, Pat Thoyts, Johannes Schindelin, Git, David Aguilar,
Junio C Hamano, Paul Mackerras
[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]
On Dec 15, 2011 8:01 PM, "Martin von Zweigbergk" <
martin.von.zweigbergk@gmail.com> wrote:
>
> Hi,
>
> On Thu, Dec 15, 2011 at 1:24 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Wed, 14 Dec 2011, David Aguilar wrote:
> >
> >> On Tue, Dec 13, 2011 at 8:50 AM, Pat Thoyts
> >> <patthoyts@users.sourceforge.net> wrote:
> >> > Launching 'gitk -- .' or 'gitk -- ..\t' restricts the display to
files
> >> > under the given directory but the file list is left empty. This is
because
> >> > the path_filter function fails to match the filenames which are
relative
> >> > to the working tree to the filter which is filessytem relative.
> >> > This solves the problem by making both names fully qualified
filesystem
> >> > paths before performing the comparison.
>
> How is this related to my patches from April? See
> http://thread.gmane.org/gmane.comp.version-control.git/170853. It's
> clearly not the same problem, but will the patches conflict? Will some
> of mine be unnecessary?
>
> > Thanks for reminding me that I did not yet apply and push. Did so now.
>
> What do you mean by this? Push to where?
> git://git.kernel.org/pub/scm/gitk/gitk.git is still down.
This is about msysgit as I also posted this there.
> Paul and Junio, the patches I sent in April are still not in git.git,
> are they? Can we use another repo until the kernel.org one is up? More
> than eight months to get a patch (or eight) merged is way too long,
> IMO.
>
> Martin
I'm not sure how this might relate to your patches. I've got a version
merged on top of the last version of the gitk report that I have which
includes those and it seems fine. As stated I did this work against
git-core as the gitk repository continues to be unavailable. However as I
have a pretty current snapshot I have pushed this to github to provide
some visibility of things I know are not present within git-core. See
http://github.com/patthoyts/gitk.git
On Dec 15, 2011 8:01 PM, "Martin von Zweigbergk" <
martin.von.zweigbergk@gmail.com> wrote:
[-- Attachment #2: Type: text/html, Size: 2962 bytes --]
^ permalink raw reply
* Re: [PATCH 10/10] revert: report fine-grained error messages from insn parser
From: Junio C Hamano @ 2011-12-15 21:23 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323881677-11117-11-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Missing space after valid action prefix
I think these two are the same and shouldn't result in different error
messages, i.e. the first one in this sequence is still an "Unrecognized
action" error and does not have anything to do with "pick" action.
pickle rr/cherry-pick~4
pick rr/cherry-pick~3
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Jonathan Nieder @ 2011-12-15 21:29 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Joey Hess
In-Reply-To: <1323967528-10537-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> The git-bundle builtin currently parses command-line options by hand;
> this is fragile, and reports cryptic errors on failure. Use the
> parse-options library to do the parsing instead.
I don't understand how this is fragile. I haven't actually run into
error messages from "git bundle" I found to be cryptic, but if they
are, they surely can be improved locally. Could you give an example
or something?
> Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>
No, not encouraged.
But parseoptification does have some nice benefits, so let's see how
the patch looks...
[...]
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> builtin/bundle.c | 91 +++++++++++++++++++++++++++++++----------------------
> t/t5704-bundle.sh | 2 +-
> 2 files changed, 54 insertions(+), 39 deletions(-)
>
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 92a8a60..13ed770 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
[...]
> @@ -9,57 +10,71 @@
[...]
> int cmd_bundle(int argc, const char **argv, const char *prefix)
> {
> - struct bundle_header header;
> - const char *cmd, *bundle_file;
> + int prefix_length;
> int bundle_fd = -1;
> - char buffer[PATH_MAX];
> - if (argc < 3)
> - usage(builtin_bundle_usage);
> + const char *subcommand, *bundle_file;
> + struct bundle_header header;
> + struct option options[] = { OPT_END() };
> +
> + argc = parse_options(argc, argv, prefix, options,
> + builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);
No, just no. Using parse-options with an empty option table is
complete overkill for handling the "-h" option. Without a lot more
justification, this doesn't make it seem more sane or readable at all.
Stopping here. I wouldn't mind seeing "git bundle" being
parseoptified, but not if the result looks like this.
I _do_ think that a systematic option-parsing library that handles
subcommands would be something possible and probably useful for git.
Its input might include a table with subcommand names, an option table
for each, and a function to call when that subcommand is used:
struct parseopt_subcommand subcmds[] = {
{ "list", no_options, notes_list },
{ "add", add_options, notes_add },
{ "copy", copy_options, notes_copy },
{ "append", append_options, notes_append },
{ "edit", no_options, notes_edit },
{ "show", no_options, notes_show },
...
};
Then "git notes -h" might be able to automatically generate a table of
synopses, and "gite notes --help-all" might print help for all
subcommands. Something like that.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/2] Improve git-bundle builtin
From: Junio C Hamano @ 2011-12-15 21:30 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder, Joey Hess
In-Reply-To: <1323967528-10537-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> A couple of thoughts:
>
> 1. There's a SP between the OBJID and the ref name in list-heads as
> opposed to the TAB used by other git commands such as ls-remote,
> diff-tree. Will fixing it break someone's parser somewhere?
If somebody will get broken when you change something, that change is not
"fixing" but just "changing". Why do you even want to "fix" it?
> 2. Is it worth fixing the "--stdin" tests? What is the usecase?
Having to pack too many refs that would not fit on a command line length
limit, which is not an unusual requirement.
^ permalink raw reply
* Re: [msysGit] [PATCH] gitk: fix the display of files when filtered by path
From: Pat Thoyts @ 2011-12-15 21:33 UTC (permalink / raw)
To: Martin von Zweigbergk
Cc: Johannes Schindelin, Paul Mackerras, Junio C Hamano,
David Aguilar, Git, msysGit
In-Reply-To: <CAOeW2eHD7Xutf+pHDyMOo=uZC9PSFZi+aMq1Rx80iTKPFApr8A@mail.gmail.com>
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
[resending as my earlier post got bounced from vger]
>Hi,
>
>On Thu, Dec 15, 2011 at 1:24 AM, Johannes Schindelin
><Johannes.Schindelin@gmx.de> wrote:
>> Hi,
>>
>> On Wed, 14 Dec 2011, David Aguilar wrote:
>>
>>> On Tue, Dec 13, 2011 at 8:50 AM, Pat Thoyts
>>> <patthoyts@users.sourceforge.net> wrote:
>>> > Launching 'gitk -- .' or 'gitk -- ..\t' restricts the display to files
>>> > under the given directory but the file list is left empty. This is because
>>> > the path_filter function fails to match the filenames which are relative
>>> > to the working tree to the filter which is filessytem relative.
>>> > This solves the problem by making both names fully qualified filesystem
>>> > paths before performing the comparison.
>
>How is this related to my patches from April? See
>http://thread.gmane.org/gmane.comp.version-control.git/170853. It's
>clearly not the same problem, but will the patches conflict? Will some
>of mine be unnecessary?
>
>> Thanks for reminding me that I did not yet apply and push. Did so now.
>
>What do you mean by this? Push to where?
>git://git.kernel.org/pub/scm/gitk/gitk.git is still down.
>
This is for msysGit.
>Paul and Junio, the patches I sent in April are still not in git.git,
>are they? Can we use another repo until the kernel.org one is up? More
>than eight months to get a patch (or eight) merged is way too long,
>IMO.
I'm not sure how this might relate to your patches. I've got a version
merged on top of the last version of the gitk report that I have which
includes those and it seems fine. As stated I did this work against
git-core as the gitk repository continues to be unavailable. However as
I have a pretty current snapshot I have pushed this to github to provide
some visibility of things I know are not present within git-core. See
http://github.com/patthoyts/gitk.git
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: Revisiting metadata storage
From: Hilco Wijbenga @ 2011-12-15 21:40 UTC (permalink / raw)
To: Richard Hartmann; +Cc: Ronan Keryell, Git List
In-Reply-To: <CAD77+gR=SjU0Ne9jort91pdHDA=RjkTJUJmnqKBipqoGUmoL_A@mail.gmail.com>
On 14 December 2011 09:59, Richard Hartmann
<richih.mailinglist@gmail.com> wrote:
> On Tue, Dec 6, 2011 at 23:45, Ronan Keryell
> <Ronan.Keryell@hpc-project.com> wrote:
>
>> At least I'm interested and began to dig into it but I do not have a lot
>> of time to work on it...
>
> If we can agree on Perl, I can try to help. I don't think I speak
> enough Python to be of use with that.
>
> Other people who have an interest in this: Please pipe up so we can
> hammer out a rough consensus & roadmap.
I'd love to have better support for metadata (specifically
timestamps). I don't care whether it's Perl, Python, Bash, or C. I
don't think I'll be much help coding but I'd like to try.
In all honesty though, I plan to rewrite our build to use file digests
instead of timestamps. Right now every rebase means a full (and almost
completely unnecessary) rebuild. Luckily I'm using the wonderful
git-new-workdir so there is no pain when switching branches.
Once the rewrite is complete (in one or two months) Git's relentless
timestamp changes should no longer affect us as much anymore. I would
still like to get a better grip on metadata though. Git should be able
to not touch files that have not changed. But whether that's feasible
or even in scope of what you have in mind... :-)
Cheers,
Hilco
^ permalink raw reply
* Re: [PATCH] Add '-P' as a synonym for '--no-pager' in the git command
From: Junio C Hamano @ 2011-12-15 21:41 UTC (permalink / raw)
To: Joe Ratterman; +Cc: git
In-Reply-To: <1323982541-18995-1-git-send-email-jratt0@gmail.com>
Sorry, but why was I listed on the To: line?
I do not recall there was a list concensus that this is a must-have or
even good-to-have, and I am not personally interested in this. I hesitate
to accept a patch that lets a feature without clear advantage squat on a
short-and-sweet single-letter option, generally speaking.
^ 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