* Re: [PATCH] Work around sed portability issue in t8006-blame-textconv
From: Junio C Hamano @ 2012-01-06 22:53 UTC (permalink / raw)
To: Ben Walton; +Cc: git
In-Reply-To: <7vd3b0vc6h.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Ben Walton <bwalton@artsci.utoronto.ca> writes:
>
>> In test 'blame --textconv with local changes' of t8006-blame-textconv,
>> using /usr/xpg4/bin/sed on Solaris as set by SANE_TOOL_PATH, an
>> additional newline was added to the output from the 'helper' script
>> driven by git attributes.
>>
>> This was noted by sed with a message such as:
>> sed: Missing newline at end of file zero.bin.
>>
>> In turn, this was triggering a fatal error from git blame:
>> fatal: unable to read files to diff
>
> Interesting. A file with incomplete line technically is not a text file
> and sed is supposed to work on text files, so it is allowed to be picky.
>
>> Use perl -p -e instead of sed -e to work around this portability issue
>> as it will not insert the newline.
>
> I am not sure if additional newline is the problem, or the exit status
> from sed is, from your description. Your first paragraph says you will get
> output from sed but with an extra newline, and then later you said blame
> noticed an error in its attempt to read the contents. I am suspecting that
> it checked the exit status from the textconv subprocess and noticed the
> error and that is the cause of the issue, but could you clarify? IOW, I
> am suspecting that replacing "as it will not insert the newline" with "as
> it does not error out on an incomplete line" is necessary in this
> sentence.
Ping?
^ permalink raw reply
* Re: [PATCH 1/2] daemon: add tests
From: Jakub Narebski @ 2012-01-06 23:35 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Clemens Buchacher, git, Jonathan Nieder,
Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20120105025154.GA7326@sigill.intra.peff.net>
On Thu, 5 Jan 2012, Jeff King wrote:
> On Wed, Jan 04, 2012 at 06:24:16PM -0800, Jakub Narebski wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > As a side note, it looks like we just start the daemon with "git daemon
> > > &". Doesn't that create a race condition with the tests which
> > > immediately try to access it (i.e., the first test may run before the
> > > daemon actually opens the socket)?
> >
> > Hmmm... perhaps the trick that git-instaweb does for "plackup" web
> > server would be of use here, waiting for socket to be ready?
>
> It looks like it busy loops, which is kind of ugly.
Well, as far as I know you can wait for data on socket or pipe, but
you can't wait for socket to be created.
Anyway this busy-wait is not too busy, and it is better than just
adding 'sleep 1' in testsuite.
> The credential-cache helper has a similar problem. It wants to kick off
> a daemon if one is not already running, and then connect to it. So the
> daemon does:
>
> printf("ok\n");
> fclose(stdout);
>
> when it has set up the socket, and the client does:
>
> r = read_in_full(daemon.out, buf, sizeof(buf));
> if (r < 0)
> die_errno("unable to read result code from cache daemon");
> if (r != 3 || memcmp(buf, "ok\n", 3))
> die("cache daemon did not start: %.*s", r, buf);
> /* now we can connect over the socket */
>
> We could probably add a "--notify-when-ready" option to git-daemon to
> do something similar.
What would git-daemon do what it is ready? Write to socket, raise signal,
print to STDOUT / STDERR?
BTW. I wonder if it would be worth it to add something a la systemd
trick creating sockets first to git-daemon. Adding systemd support
doesn't make sense for daemon that is to be run from inetd / xinetd,
I guess.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [ANNOUNCE] Git 1.7.9-rc0
From: Philip Oakley @ 2012-01-07 0:23 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <7vr4zciji4.fsf@alter.siamese.dyndns.org>
minor spelling
From: "Junio C Hamano" <gitster@pobox.com> Sent: Friday, January 06, 2012
9:55 PM
> Git v1.7.9 Release Notes (draft)
> ========================
>
> * "git log --format='<format>'" learned new %g[nNeE] specifiers to
> show information from the reflog entries when warlking the reflog
> (i.e. with "-g").
s/warlking/walking/
^ permalink raw reply
* Re: How to deal with historic tar-balls
From: nn6eumtr @ 2012-01-07 1:10 UTC (permalink / raw)
To: Neal Kreitzinger, git
In-Reply-To: <4F05C0E2.4050101@gmail.com>
Thanks for the response, there is lots of good information there.
One clarification - can you track renames in git? I tried using git mv
but from the status output it looks like it deleted the old file and
added the new file. I was expecting it to record some sort of indicator
of the name change, instead it looks like a short-cut for delete & add,
the docs aren't clear if that is the case.
On 1/5/2012 10:25 AM, Neal Kreitzinger wrote:
...
> going to want to do appropriate clean up of the working tree in each
> iteration before committing. This is where you would review
> renames/removes with git-status before you git-add and git-commit. Also,
> if you are tracking permissions in git (the executable bit) then you
> will want to filter out any noise generated by frivolous permissions
> changes between the tarball contents.
...
^ permalink raw reply
* Re: [PATCH] Documentation: rerere.enabled overrides [ -d rr-cache ]
From: Thomas Rast @ 2012-01-07 1:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfwfsk24y.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The manual page for "rerere" talks about "configuration variable
> rerere.enabled"; perhaps it should also refer to git config manual page to
> make it more discoverable?
Maybe, but it already says you should set the variable in two different
places.
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> ... OTOH the
>> auto-creation of rr-cache can cause strange behavior if a user has
>> rerere.enabled unset and tries it once, as in
>>
>> git config rerere.enabled true
>> git merge ...
>> git config --unset rerere.enabled
>
> That is because the last one should be
>
> git config --bool rerere.enabled false
I definitely meant --unset. If the user knows the distinction, and
wants to return the variable to the state it had before his test
(perhaps so that a future --global setting might take effect), he would
use this sequence. He might then be somewhat surprised to see that
rerere is now permamently enabled for this repo.
Probably I'm worrying too much about a weird fringe case though.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: How to deal with historic tar-balls
From: Thomas Rast @ 2012-01-07 1:50 UTC (permalink / raw)
To: nn6eumtr; +Cc: Neal Kreitzinger, git
In-Reply-To: <4F079BA1.3060907@gmail.com>
nn6eumtr <nn6eumtr@gmail.com> writes:
> Thanks for the response, there is lots of good information there.
>
> One clarification - can you track renames in git? I tried using git mv
> but from the status output it looks like it deleted the old file and
> added the new file. I was expecting it to record some sort of
> indicator of the name change, instead it looks like a short-cut for
> delete & add, the docs aren't clear if that is the case.
Git only stores snapshots; so for an ordinary (non-merge, non-root)
commit, you have the "before" (parent) and "after" (commit's) snapshot.
Everything is generated on the fly from that, including diffs, heuristic
rename detection, pickaxe, ...
To apply rename detection when diffing (e.g. in diff, log, show,
format-patch), use the -M flag.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2012-01-07 4:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski
In-Reply-To: <7vipkrp9pq.fsf@alter.siamese.dyndns.org>
Hi,
Am 04.01.2012 20:08 schrieb Junio C Hamano:
> Is there a way to ask Term::ReadKey (or possibly some other module) if we
> will be able to interact with the terminal _before_ we give that prompt?
>
> The simplest would be to do this, I would think, but I didn't test it.
>
> if (!defined $ret && -t) {
> print STDERR $prompt;
> if ($isPassword) {
> ...
> }
-t does not help, but I think it's not a big deal if the prompt is printed on
the terminal and also on the ASKPASS-helper.
Using Term::ReadLine seems to help:
...
$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
}
if (!defined $ret) {
use Term::Readline;
my $term = Term::ReadLine->new("Git.pm");
if ($isPassword) {
require Term::ReadKey;
Term::ReadKey::ReadMode('noecho');
}
$ret = $term->readline($prompt);
if ($isPassword) {
Term::ReadKey::ReadMode('restore');
print STDERR "\n";
}
}
if (!defined $ret) {
$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
...
But I'm not sure if this is what we want, because you can go with the cursor
over the whole terminal.
A better (working) alternative might be:
...
$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
}
use Term::Readline;
my $term = Term::ReadLine->new("Git.pm");
if (!defined $ret && fileno($term->IN)) {
print STDERR $prompt;
if ($isPassword) {
...
}
...
--
Best regards,
Sven Strickroth
ClamAV, a GPL anti-virus toolkit http://www.clamav.net
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply
* Re: [PATCH 2/3] show_ref(): remove unused "flag" and "cb_data" arguments
From: Junio C Hamano @ 2012-01-07 5:08 UTC (permalink / raw)
To: mhagger; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <1325859153-31016-3-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> I suppose, though I didn't verify, that the old function signature was
> a vestige of its earlier having been used as a callback function. But
> it doesn't really matter; the point is that the extra arguments are
> currently not needed.
Yeah, since 8a65ff7 (Generalize the "show each ref" code in receice-pack,
2005-07-02) the function has always been fed to for_each_ref(), but when
6b01ecf (ref namespaces: Support remote repositories via upload-pack and
receive-pack, 2011-07-08) introduced show_ref_cb() as the callback that
uses show_ref() as a helper, it forgot to do the clean-up in this patch.
^ permalink raw reply
* Re: Aborting "git commit --interactive" discards updates to index
From: Junio C Hamano @ 2012-01-07 5:08 UTC (permalink / raw)
To: demerphq; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <CANgJU+X+qLe3Aqy_ZpoSDKMuf=8=OxVvpkt0tGmSi=KVgti3HQ@mail.gmail.com>
demerphq <demerphq@gmail.com> writes:
> On 27 June 2011 17:59, Junio C Hamano <gitster@pobox.com> wrote:
>> The latest feature release Git 1.7.6 is available at the usual
>> places:
>>
>> http://www.kernel.org/pub/software/scm/git/
> [snip]
>> * Aborting "git commit --interactive" discards updates to the index
>> made during the interactive session.
>
> Hi, I am wondering why this change was made?
I wasn't directly involved in this particular part of the design of what
should happen to the index when a commit is aborted, so I would be a bad
person to give you the first answer, but let's try.
If a "commit" session is aborted, it is logical to revert whatever has
been done inside that session as a single logical unit, so I do not
particularly find the current behaviour so confusing. It might even make
more sense if we update "commit -i" and "commit -a" to also revert the
index modification when the command is aborted for consistency.
You are welcome to rehash the age old discussion, though. Personally I do
not care very deeply either way. I would never use "commit --interactive"
myself, and I would not encourage others to use it, either, even if we do
not worry about the behaviour when a commit is aborted.
One thread of interest (there are others, as this change was rerolled at
least a few times) may be
http://thread.gmane.org/gmane.comp.version-control.git/173033/focus=173035
Having said all that,...
> .... I am writing this after spending about 45 minutes showing a
> colleague how to use git commit --interactive, when we realized that
> we had forgotten to add a file....
... if your partial commit is so complex that you need to spend 45 minutes
to sift what to be commited and what to be left out, you are much better
off to run "git add -i" to prepare the index, "git stash save -k" to check
out what is to be committed (and stash away what are to be left out) so
that you can make sure what you are committing is what you thought are
committing (by asking "git diff" and "make test" for example), and after
convincing yourself that you made a good state in the index, make a commit
with "git commit" (without any other arguments) and conclude it with "git
stash pop" to recover the changes that you decided to leave out.
"commit --interactive" robs me from that crucial "verification" step, and
that is why I wouldn't be a user or an advocate of this "misfeature".
By the way, why did you draw Ævar into this discussion? I do not think he
was involved in any way with the design or implementation of this command.
^ permalink raw reply
* Re: [PATCH] Documentation: rerere.enabled overrides [ -d rr-cache ]
From: Junio C Hamano @ 2012-01-07 5:17 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git
In-Reply-To: <87boqge19s.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The manual page for "rerere" talks about "configuration variable
>> rerere.enabled"; perhaps it should also refer to git config manual page to
>> make it more discoverable?
>
> Maybe, but it already says you should set the variable in two different
> places.
That is not the point.
The documentation for git config seems to be the only place where we
explain that the existence of rr-cache determines what happens when the
user does _not_ set the variable; lack of that description will lead to
the confusion you describe below:
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> ... OTOH the
>>> auto-creation of rr-cache can cause strange behavior if a user has
>>> rerere.enabled unset and tries it once, as in
>>>
>>> git config rerere.enabled true
>>> git merge ...
>>> git config --unset rerere.enabled
>>
>> That is because the last one should be
>>
>> git config --bool rerere.enabled false
>
> I definitely meant --unset. If the user knows the distinction, and
> wants to return the variable to the state it had before his test ...
Running "unset" is how to return the _variable_ to the previous state, but
that is _not_ how to return to the previous state of the _repository_.
Perhaps something like this in addition?
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 04f5e19..c523c67 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1786,7 +1786,8 @@ rerere.enabled::
conflict hunks can be resolved automatically, should they be
encountered again. By default, linkgit:git-rerere[1] is
enabled if there is an `rr-cache` directory under the
- `$GIT_DIR`.
+ `$GIT_DIR`, e.g. if "rerere" was previously used in the
+ repository.
sendemail.identity::
A configuration identity. When given, causes values in the
^ permalink raw reply related
* [PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects
From: Jakub Narebski @ 2012-01-07 10:47 UTC (permalink / raw)
To: git
When gitweb URL does not provide action explicitly, e.g.
http://git.example.org/repo.git/branch
dispatch() tries to guess action (view to be used) based on remaining
parameters. Among others it is based on the type of requested object,
which gave problems when asking for non-existent branch or file (for
example misspelt name).
Now undefined $action from dispatch() should not result in problems.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm not sure if this is the way to fix it, by erroring-out in
dispatch() and leaving $action undefined.
Testsuite passes, but I have not examined output intensively.
gitweb/gitweb.perl | 4 +++-
t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f884dfe..e2e04df 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1123,8 +1123,10 @@ sub dispatch {
if (!defined $action) {
if (defined $hash) {
$action = git_get_type($hash);
+ $action or die_error(404, "Object does not exist");
} elsif (defined $hash_base && defined $file_name) {
$action = git_get_type("$hash_base:$file_name");
+ $action or die_error(404, "File or directory does not exist");
} elsif (defined $project) {
$action = 'summary';
} else {
@@ -2391,7 +2393,7 @@ sub get_feed_info {
return unless (defined $project);
# some views should link to OPML, or to generic project feed,
# or don't have specific feed yet (so they should use generic)
- return if ($action =~ /^(?:tags|heads|forks|tag|search)$/x);
+ return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
my $branch;
# branches refs uses 'refs/heads/' prefix (fullname) to differentiate
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index ab24917..0f771c6 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -475,6 +475,14 @@ test_expect_success \
'gitweb_run "" "/.git/master:foo/"'
test_expect_success \
+ 'path_info: project/branch (non-existent)' \
+ 'gitweb_run "" "/.git/non-existent"'
+
+test_expect_success \
+ 'path_info: project/branch:filename (non-existent branch)' \
+ 'gitweb_run "" "/.git/non-existent:non-existent"'
+
+test_expect_success \
'path_info: project/branch:file (non-existent)' \
'gitweb_run "" "/.git/master:non-existent"'
^ permalink raw reply related
* [PATCH 2/5] run-command: kill children on exit by default
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-1-git-send-email-drizzd@aon.at>
It feels natural for a user to view git commands as monolithic
commands with a single thread of execution. If the parent git
command dies, it should therefore clean up its child processes as
well. So enable the cleanup mechanism by default.
For dashed externals, this means that killing the git wrapper will
kill the command itself, just like what would happen in case of an
internal command. A notable exception is the credentials cache
daemon, which must stay alive after the store command has
completed.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
I considered squashing this into the previous commit. But it's a fairly
small change and may help with bisecting in case of problems.
credential-cache.c | 1 +
run-command.c | 4 ++--
run-command.h | 2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..15e7236 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -48,6 +48,7 @@ static void spawn_daemon(const char *socket)
daemon.argv = argv;
daemon.no_stdin = 1;
daemon.out = -1;
+ daemon.stay_alive_on_exit = 1;
if (start_command(&daemon))
die_errno("unable to start cache daemon");
diff --git a/run-command.c b/run-command.c
index 0204aaf..fe07b20 100644
--- a/run-command.c
+++ b/run-command.c
@@ -353,7 +353,7 @@ fail_pipe:
if (cmd->pid < 0)
error("cannot fork() for %s: %s", cmd->argv[0],
strerror(failed_errno = errno));
- else if (cmd->clean_on_exit)
+ else if (!cmd->stay_alive_on_exit)
mark_child_for_cleanup(cmd->pid);
/*
@@ -420,7 +420,7 @@ fail_pipe:
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
- if (cmd->clean_on_exit && cmd->pid >= 0)
+ if (!cmd->stay_alive_on_exit && cmd->pid >= 0)
mark_child_for_cleanup(cmd->pid);
if (cmd->env)
diff --git a/run-command.h b/run-command.h
index 2a69466..69dbea1 100644
--- a/run-command.h
+++ b/run-command.h
@@ -38,7 +38,7 @@ struct child_process {
unsigned silent_exec_failure:1;
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
- unsigned clean_on_exit:1;
+ unsigned stay_alive_on_exit:1;
void (*preexec_cb)(void);
};
--
1.7.8
^ permalink raw reply related
* [PATCH 4/5] git-daemon: produce output when ready
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-1-git-send-email-drizzd@aon.at>
If a client tries to connect after git-daemon starts, but before it
opens a listening socket, the connection will fail. Output "[PID]
Ready to rumble]" after opening the socket successfully in order to
inform the user that the daemon is now ready to receive
connections.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
daemon.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/daemon.c b/daemon.c
index 15ce918..ab21e66 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1086,6 +1086,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
drop_privileges(cred);
+ loginfo("Ready to rumble");
+
return service_loop(&socklist);
}
@@ -1270,10 +1272,8 @@ int main(int argc, char **argv)
if (inetd_mode || serve_mode)
return execute();
- if (detach) {
+ if (detach)
daemonize();
- loginfo("Ready to rumble");
- }
else
sanitize_stdfds();
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/2] daemon: add tests
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vipkoih0e.fsf@alter.siamese.dyndns.org>
On Fri, Jan 06, 2012 at 02:49:05PM -0800, Junio C Hamano wrote:
>
> but it seems that the best course of action would be to drop it
> and queue your re-roll afresh, aiming for the next cycle.
Here's the re-rolled series, also available as cb/git-daemon-tests based
on current master at https://github.com/drizzd/git .
[PATCH 1/5] run-command: optionally kill children on exit
[PATCH 2/5] run-command: kill children on exit by default
[PATCH 3/5] git-daemon: add tests
[PATCH 4/5] git-daemon: produce output when ready
[PATCH 5/5] git-daemon tests: wait until daemon is ready
On Fri, Jan 06, 2012 at 05:32:15PM -0500, Jeff King wrote:
> On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:
>
> > I have rebased Junio's cb/git-daemon-tests onto your
> > jk/child-cleanup and replaced the call to pkill with a regular kill
> > command.
>
> Looks pretty good from my cursory examination. I think you should fill
> out the rationale for "kill dashed externals on exit" a bit. My
> reasoning is that whether a git command is an internal or external
> process is purely an implementation detail, and killing the git wrapper
> should behave identically in both cases.
The previous version of this patch only changed the behavior for users
of run_command_v_opt, but not for those who filled out the child_process
structure by themselves. I could have manually enabled all of those, but
that felt unnatural. Instead, I have now reversed the meaning of
clean_on_exit to stay_alive_on_exit in [PATCH 2/5] run-command: kill
children on exit by default. Cleanup is on by default and callers of
run_command must disable it if children should stay alive.
^ permalink raw reply
* [PATCH 5/5] git-daemon tests: wait until daemon is ready
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-1-git-send-email-drizzd@aon.at>
In start_daemon, git-daemon is started as a background process. In
theory, the tests may try to connect before the daemon had a chance
to open a listening socket. Avoid this race condition by waiting
for it to output "Ready to rumble". Any other output is considered
an error and the test is aborted.
Should git-daemon produce no output at all, lib-git-daemon would
block forever. This could be fixed by introducing a timeout. On
the other hand, we have no timeout for other git commands which
could suffer from the same problem. Since such a mechanism adds
some complexity, I have decided against it.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/lib-git-daemon.sh | 18 +++++++++++++++++-
1 files changed, 17 insertions(+), 1 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 5e81a25..ef2d01f 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -23,12 +23,27 @@ start_git_daemon() {
trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
say >&3 "Starting git daemon ..."
+ mkfifo git_daemon_output
git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
--reuseaddr --verbose \
--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
- >&3 2>&4 &
+ >&3 2>git_daemon_output &
GIT_DAEMON_PID=$!
+ {
+ read line
+ echo >&4 "$line"
+ cat >&4 &
+
+ # Check expected output
+ if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
+ then
+ kill "$GIT_DAEMON_PID"
+ wait "$GIT_DAEMON_PID"
+ trap 'die' EXIT
+ error "git daemon failed to start"
+ fi
+ } <git_daemon_output
}
stop_git_daemon() {
@@ -50,4 +65,5 @@ stop_git_daemon() {
error "git daemon exited with status: $ret"
fi
GIT_DAEMON_PID=
+ rm -f git_daemon_output
}
--
1.7.8
^ permalink raw reply related
* [PATCH 1/5] run-command: optionally kill children on exit
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-1-git-send-email-drizzd@aon.at>
From: Jeff King <peff@peff.net>
When we spawn a helper process, it should generally be done
and finish_command called before we exit. However, if we
exit abnormally due to an early return or a signal, the
helper may continue to run in our absence.
In the best case, this may simply be wasted CPU cycles or a
few stray messages on a terminal. But it could also mean a
process that the user thought was aborted continues to run
to completion (e.g., a push's pack-objects helper will
complete the push, even though you killed the push process).
This patch provides infrastructure for run-command to keep
track of PIDs to be killed, and clean them on signal
reception or input, just as we do with tempfiles. PIDs can
be added in two ways:
1. If NO_PTHREADS is defined, async helper processes are
automatically marked. By definition this code must be
ready to die when the parent dies, since it may be
implemented as a thread of the parent process.
2. If the run-command caller specifies the "clean_on_exit"
option. This is not the default, as there are cases
where it is OK for the child to outlive us (e.g., when
spawning a pager).
PIDs are cleared from the kill-list automatically during
wait_or_whine, which is called from finish_command and
finish_async.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Not sure if I can sign off without your sign-off. Should I have
replaced this with Acked-by?
run-command.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
run-command.h | 1 +
2 files changed, 69 insertions(+), 0 deletions(-)
diff --git a/run-command.c b/run-command.c
index 1c51043..0204aaf 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1,8 +1,66 @@
#include "cache.h"
#include "run-command.h"
#include "exec_cmd.h"
+#include "sigchain.h"
#include "argv-array.h"
+struct child_to_clean {
+ pid_t pid;
+ struct child_to_clean *next;
+};
+static struct child_to_clean *children_to_clean;
+static int installed_child_cleanup_handler;
+
+static void cleanup_children(int sig)
+{
+ while (children_to_clean) {
+ struct child_to_clean *p = children_to_clean;
+ children_to_clean = p->next;
+ kill(p->pid, sig);
+ free(p);
+ }
+}
+
+static void cleanup_children_on_signal(int sig)
+{
+ cleanup_children(sig);
+ sigchain_pop(sig);
+ raise(sig);
+}
+
+static void cleanup_children_on_exit(void)
+{
+ cleanup_children(SIGTERM);
+}
+
+static void mark_child_for_cleanup(pid_t pid)
+{
+ struct child_to_clean *p = xmalloc(sizeof(*p));
+ p->pid = pid;
+ p->next = children_to_clean;
+ children_to_clean = p;
+
+ if (!installed_child_cleanup_handler) {
+ atexit(cleanup_children_on_exit);
+ sigchain_push_common(cleanup_children_on_signal);
+ installed_child_cleanup_handler = 1;
+ }
+}
+
+static void clear_child_for_cleanup(pid_t pid)
+{
+ struct child_to_clean **last, *p;
+
+ last = &children_to_clean;
+ for (p = children_to_clean; p; p = p->next) {
+ if (p->pid == pid) {
+ *last = p->next;
+ free(p);
+ return;
+ }
+ }
+}
+
static inline void close_pair(int fd[2])
{
close(fd[0]);
@@ -130,6 +188,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
} else {
error("waitpid is confused (%s)", argv0);
}
+
+ clear_child_for_cleanup(pid);
+
errno = failed_errno;
return code;
}
@@ -292,6 +353,8 @@ fail_pipe:
if (cmd->pid < 0)
error("cannot fork() for %s: %s", cmd->argv[0],
strerror(failed_errno = errno));
+ else if (cmd->clean_on_exit)
+ mark_child_for_cleanup(cmd->pid);
/*
* Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -312,6 +375,7 @@ fail_pipe:
cmd->pid = -1;
}
close(notify_pipe[0]);
+
}
#else
{
@@ -356,6 +420,8 @@ fail_pipe:
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
+ if (cmd->clean_on_exit && cmd->pid >= 0)
+ mark_child_for_cleanup(cmd->pid);
if (cmd->env)
free_environ(env);
@@ -540,6 +606,8 @@ int start_async(struct async *async)
exit(!!async->proc(proc_in, proc_out, async->data));
}
+ mark_child_for_cleanup(async->pid);
+
if (need_in)
close(fdin[0]);
else if (async->in)
diff --git a/run-command.h b/run-command.h
index 56491b9..2a69466 100644
--- a/run-command.h
+++ b/run-command.h
@@ -38,6 +38,7 @@ struct child_process {
unsigned silent_exec_failure:1;
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
+ unsigned clean_on_exit:1;
void (*preexec_cb)(void);
};
--
1.7.8
^ permalink raw reply related
* [PATCH 3/5] git-daemon: add tests
From: Clemens Buchacher @ 2012-01-07 11:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-1-git-send-email-drizzd@aon.at>
The semantics of the git daemon tests are similar to the http transport
tests. In fact, they are only a slightly modified copy of t5550, plus the
newly added remote error tests.
All git-daemon tests will be skipped unless the environment variable
GIT_TEST_GIT_DAEMON is set.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/lib-git-daemon.sh | 53 +++++++++++++++++
t/t5570-git-daemon.sh | 148 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 201 insertions(+), 0 deletions(-)
create mode 100644 t/lib-git-daemon.sh
create mode 100755 t/t5570-git-daemon.sh
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
new file mode 100644
index 0000000..5e81a25
--- /dev/null
+++ b/t/lib-git-daemon.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_GIT_DAEMON"
+then
+ skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)"
+ test_done
+fi
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}
+
+GIT_DAEMON_PID=
+GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT
+
+start_git_daemon() {
+ if test -n "$GIT_DAEMON_PID"
+ then
+ error "start_git_daemon already called"
+ fi
+
+ mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
+
+ trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
+
+ say >&3 "Starting git daemon ..."
+ git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+ --reuseaddr --verbose \
+ --base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+ "$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+ >&3 2>&4 &
+ GIT_DAEMON_PID=$!
+}
+
+stop_git_daemon() {
+ if test -z "$GIT_DAEMON_PID"
+ then
+ return
+ fi
+
+ trap 'die' EXIT
+
+ # kill git-daemon child of git
+ say >&3 "Stopping git daemon ..."
+ kill "$GIT_DAEMON_PID"
+ wait "$GIT_DAEMON_PID" >&3 2>&4
+ ret=$?
+ # expect exit with status 143 = 128+15 for signal TERM=15
+ if test $ret -ne 143
+ then
+ error "git daemon exited with status: $ret"
+ fi
+ GIT_DAEMON_PID=
+}
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..7cbc999
--- /dev/null
+++ b/t/t5570-git-daemon.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+start_git_daemon
+
+test_expect_success 'setup repository' '
+ echo content >file &&
+ git add file &&
+ git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+ mkdir "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+ (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+ git --bare init &&
+ : >git-daemon-export-ok
+ ) &&
+ git remote add public "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+ git clone "$GIT_DAEMON_URL/repo.git" clone &&
+ test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+ echo content >>file &&
+ git commit -a -m two &&
+ git push public &&
+ (cd clone && git pull) &&
+ test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+ git push public master:other &&
+ (cd clone &&
+ git remote set-head -d origin &&
+ git remote set-head -a origin &&
+ git symbolic-ref refs/remotes/origin/HEAD > output &&
+ echo refs/remotes/origin/master > expect &&
+ test_cmp expect output
+ )
+'
+
+test_expect_success 'prepare pack objects' '
+ cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ git --bare repack -a -d
+ )
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+ cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+ (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+ p=`ls objects/pack/pack-*.pack` &&
+ chmod u+w $p &&
+ printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+ ) &&
+ mkdir repo_bad1.git &&
+ (cd repo_bad1.git &&
+ git --bare init &&
+ test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad1.git" &&
+ test 0 = `ls objects/pack/pack-*.pack | wc -l`
+ )
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+ cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+ (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+ p=`ls objects/pack/pack-*.idx` &&
+ chmod u+w $p &&
+ printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+ ) &&
+ mkdir repo_bad2.git &&
+ (cd repo_bad2.git &&
+ git --bare init &&
+ test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad2.git" &&
+ test 0 = `ls objects/pack | wc -l`
+ )
+'
+
+test_remote_error()
+{
+ do_export=YesPlease
+ while test $# -gt 0
+ do
+ case $1 in
+ -x)
+ shift
+ chmod -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+ ;;
+ -n)
+ shift
+ do_export=
+ ;;
+ *)
+ break
+ esac
+ done
+
+ if test $# -ne 3
+ then
+ error "invalid number of arguments"
+ fi
+
+ cmd=$1
+ repo=$2
+ msg=$3
+
+ if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
+ then
+ if test -n "$do_export"
+ then
+ : >"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+ else
+ rm -f "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+ fi
+ fi
+
+ test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+ echo "fatal: remote error: $msg: /$repo" >expect &&
+ test_cmp expect output
+ ret=$?
+ chmod +x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+ (exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git '$msg'"
+test_expect_success 'push disabled' "test_remote_error push repo.git '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git '$msg'"
+test_expect_success 'not exported' "test_remote_error -n fetch repo.git '$msg'"
+
+stop_git_daemon
+start_git_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled' "test_remote_error push repo.git 'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'no such repository'"
+test_expect_success 'not exported' "test_remote_error -n fetch repo.git 'repository not exported'"
+
+stop_git_daemon
+test_done
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/2] daemon: add tests
From: Clemens Buchacher @ 2012-01-07 11:46 UTC (permalink / raw)
To: Jakub Narebski
Cc: Jeff King, Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <201201070035.52581.jnareb@gmail.com>
On Sat, Jan 07, 2012 at 12:35:50AM +0100, Jakub Narebski wrote:
> >
> > We could probably add a "--notify-when-ready" option to git-daemon to
> > do something similar.
>
> What would git-daemon do what it is ready? Write to socket, raise signal,
> print to STDOUT / STDERR?
Please have a look at my "git-daemon: produce output when ready" patch.
After opening the socket, git-daemon --verbose writes "Ready to rumble"
to stderr.
^ permalink raw reply
* [PATCH] credentials: unable to connect to cache daemon
From: Clemens Buchacher @ 2012-01-07 11:54 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120106223215.GA13106@sigill.intra.peff.net>
Error out if we just spawned the daemon and yet we cannot connect.
And always release the string buffer.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Hi Jeff,
I wrote this while debugging why t0301-credential-cache.sh failed after
I enabled cleanup_children by default. This error condition turned out
not to be the problem, and this patch would not have helped in debugging
this case. But I think it makes sense anyways.
credential-cache.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..8f25c06 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -71,11 +71,10 @@ static void do_cache(const char *socket, const char *action, int timeout,
die_errno("unable to relay credential");
}
- if (!send_request(socket, &buf))
- return;
- if (flags & FLAG_SPAWN) {
+ if (send_request(socket, &buf) < 0 && flags & FLAG_SPAWN) {
spawn_daemon(socket);
- send_request(socket, &buf);
+ if (send_request(socket, &buf) < 0)
+ die_errno("unable to connect to cache daemon");
}
strbuf_release(&buf);
}
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/5] run-command: optionally kill children on exit
From: Erik Faye-Lund @ 2012-01-07 12:45 UTC (permalink / raw)
To: Clemens Buchacher
Cc: Junio C Hamano, git, Jeff King, Jonathan Nieder, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-2-git-send-email-drizzd@aon.at>
On Sat, Jan 7, 2012 at 12:42 PM, Clemens Buchacher <drizzd@aon.at> wrote:
> +static void cleanup_children(int sig)
> +{
> + while (children_to_clean) {
> + struct child_to_clean *p = children_to_clean;
> + children_to_clean = p->next;
> + kill(p->pid, sig);
> + free(p);
> + }
> +}
> +
> +static void cleanup_children_on_signal(int sig)
> +{
> + cleanup_children(sig);
> + sigchain_pop(sig);
> + raise(sig);
> +}
> +
Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
supports SIGKILL, so propagating other signals to child-processes will
fail with EINVAL. That being said, Windows' support for signals is
severely limited, but I'm not entirely sure which ones can be
generated in this case.
> @@ -312,6 +375,7 @@ fail_pipe:
> cmd->pid = -1;
> }
> close(notify_pipe[0]);
> +
> }
> #else
> {
This hunk is probably unintentional...
^ permalink raw reply
* Re: Aborting "git commit --interactive" discards updates to index
From: demerphq @ 2012-01-07 14:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ævar Arnfjörð
In-Reply-To: <7vvcoogkw8.fsf@alter.siamese.dyndns.org>
On 7 January 2012 06:08, Junio C Hamano <gitster@pobox.com> wrote:
> demerphq <demerphq@gmail.com> writes:
>
>> On 27 June 2011 17:59, Junio C Hamano <gitster@pobox.com> wrote:
>>> The latest feature release Git 1.7.6 is available at the usual
>>> places:
>>>
>>> http://www.kernel.org/pub/software/scm/git/
>> [snip]
>>> * Aborting "git commit --interactive" discards updates to the index
>>> made during the interactive session.
>>
>> Hi, I am wondering why this change was made?
>
> I wasn't directly involved in this particular part of the design of what
> should happen to the index when a commit is aborted, so I would be a bad
> person to give you the first answer, but let's try.
>
> If a "commit" session is aborted, it is logical to revert whatever has
> been done inside that session as a single logical unit, so I do not
> particularly find the current behaviour so confusing. It might even make
> more sense if we update "commit -i" and "commit -a" to also revert the
> index modification when the command is aborted for consistency.
>
> You are welcome to rehash the age old discussion, though. Personally I do
> not care very deeply either way. I would never use "commit --interactive"
> myself, and I would not encourage others to use it, either, even if we do
> not worry about the behaviour when a commit is aborted.
If I were to provide a patch to make this behavior configurable would
you have any objections? I personally much prefer the old behaviour. I
want it to leave the stuff in my index.
>
> One thread of interest (there are others, as this change was rerolled at
> least a few times) may be
>
> http://thread.gmane.org/gmane.comp.version-control.git/173033/focus=173035
Thanks.
> Having said all that,...
>
>> .... I am writing this after spending about 45 minutes showing a
>> colleague how to use git commit --interactive, when we realized that
>> we had forgotten to add a file....
>
> ... if your partial commit is so complex that you need to spend 45 minutes
> to sift what to be commited and what to be left out, you are much better
I was showing a colleague how to use the features it provides on a
large patch where the committer wanted to keep various bits and not
keep others.
> off to run "git add -i" to prepare the index, "git stash save -k" to check
> out what is to be committed (and stash away what are to be left out) so
> that you can make sure what you are committing is what you thought are
> committing (by asking "git diff" and "make test" for example), and after
Isnt this what the diff option in commit interactive is for?
Personally I tend to review patches in detail before I push them, not
necessarily before I commit them.
> convincing yourself that you made a good state in the index, make a commit
> with "git commit" (without any other arguments) and conclude it with "git
> stash pop" to recover the changes that you decided to leave out.
I personally have never had an issue with git commit --interactive so
this procedure seems really convoluted to me, and a lot harder to
teach to a colleague than "use git commit --interactive". Is there a
real problem it solves?
> "commit --interactive" robs me from that crucial "verification" step, and
> that is why I wouldn't be a user or an advocate of this "misfeature".
I understand. But that is a workflow directed to statically testable
library code. Our workflow doesn't really depend on a "make test"
phase. Also, if I calling git commit --interactive (or git add -i),
then I am as confident my code works as I can be, and the reason I am
doing an interactive step is for instance to edit out debug lines, or
separate out whitespace changes from code changes, or to break my
change set into several logical groups.
> By the way, why did you draw Ævar into this discussion? I do not think he
> was involved in any way with the design or implementation of this command.
He is a git hacker, and is a friend and colleague. We both work for a
largish dotcom which uses git as our primary version control and we
have collaborated on a tool I wrote to use git to manage our rollout
processes. So when something git comes up it is natural to me to CC
him.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v3] Limit refs to fetch to minimum in shallow clones
From: Nguyen Thai Ngoc Duy @ 2012-01-07 14:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn O. Pearce
In-Reply-To: <7vfwfsllny.fsf@alter.siamese.dyndns.org>
2012/1/7 Junio C Hamano <gitster@pobox.com>:
> My first reaction after reading "is implied by default" was "Huh? didn't
> we just read these kick in only when --depth is given?" and I had to read
> it again. Here is my attempt to rephrase it.
>
> Clone only the history leading to the tip of a single branch,
> either specified by the `--branch` option or the primary branch
> remote's `HEAD` points at. When creating a shallow clone with the
> `--depth` option, this is the default, unless `--no-single-branch`
> is given to fetch the histories near the tips of all branches.
>
> Currently this option only works when creating a shallow clone and
> does not have any effect without the `--depth` option.
>
> We might want to later enhance this to work also with a full-depth clone
> that tracks only one branch, and the above phrasing would make it clear.
Interesting. Yes that's another possibility.
>> + if (!option_branch)
>> + remote_head = guess_remote_head(head, refs, 0);
>> + else {
>> + struct strbuf sb = STRBUF_INIT;
>> + strbuf_addstr(&sb, src_ref_prefix);
>> + strbuf_addstr(&sb, option_branch);
>> + remote_head = find_ref_by_name(refs, sb.buf);
>> + strbuf_release(&sb);
>> + }
>> +
>> + if (!remote_head)
>> + die(_("Remote branch \"%s\" not found. Nothing to clone.\n"
>> + "Try --no-single-branch to fetch all refs."),
>> + option_branch ? option_branch : "HEAD");
>
> Switching upon option_branch to tweak the message is a good idea, but
> strictly speaking, we would hit this die() when guess_remote_head() does
> not find where HEAD points at because it is detached, and in that case,
> the error is not "Nothing to clone", but "We couldn't tell which branch
> you meant to limit this cloning to".
Yeah. And in detached case without --branch, we probably should not
say anything.
>> @@ -642,9 +679,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>>
>> - if (option_depth)
>> + if (option_depth) {
>> transport_set_option(transport, TRANS_OPT_DEPTH,
>> option_depth);
>> + transport_set_option(transport, TRANS_OPT_FOLLOWTAGS,
>> + option_single_branch ? "1" : NULL);
>
> Curious. Does anybody set FOLLOWTAGS to the transport by default becore we
> come here (just asking)?
No, I just hate another "if (option_single_branch)", which indents the
call one more time. Always setting it to "1" in this case should
probably be OK too. I'm just not sure if upload-pack realizes all tags
are requested so include-tag extension means nothing, or it does extra
work for no gain.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/5] run-command: optionally kill children on exit
From: Jeff King @ 2012-01-07 14:41 UTC (permalink / raw)
To: Clemens Buchacher
Cc: Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-2-git-send-email-drizzd@aon.at>
On Sat, Jan 07, 2012 at 12:42:43PM +0100, Clemens Buchacher wrote:
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> Not sure if I can sign off without your sign-off. Should I have
> replaced this with Acked-by?
Sorry, I usually sign-off when I sent to the list. But:
Signed-off-by: Jeff King <peff@peff.net>
for this and the other patch in this series.
As for whether you can sign-off, I think it is OK in this case. You are
basically signing off on the "Certificate of Origin" found in
SubmittingPatches. I think you are covered under (b), which is that to
the best of your knowledge it is based on open source work (i.e., even
though I didn't sign off explicitly, it is pretty obvious that this is
meant to be open source). But it's nicer to be explicit.
-Peff
^ permalink raw reply
* [PATCH v4] clone: add --single-branch to fetch only one branch
From: Nguyễn Thái Ngọc Duy @ 2012-01-07 14:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn O. Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1325833869-20078-1-git-send-email-pclouds@gmail.com>
When --single-branch is given, only one branch, either HEAD or one
specified by --branch, will be fetched. Also only tags that point to
the downloaded history are fetched.
This helps most in shallow clones, where it can reduce the download to
minimum and that is why it is enabled by default when --depth is given.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
The fourth round does not limit --single-branch to shallow clones any
more. I did not see that the patch finally comes close to what Carlos
tried to do [1], the patch that reminded me of the tag issue in shallow
clone Shawn mentioned a while ago.
[1] http://thread.gmane.org/gmane.comp.version-control.git/187639
Documentation/git-clone.txt | 11 ++++++-
builtin/clone.c | 52 ++++++++++++++++++++++++++++--
t/t5500-fetch-pack.sh | 72 ++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 129 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 4b8b26b..0931a3e 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -13,7 +13,8 @@ SYNOPSIS
[-l] [-s] [--no-hardlinks] [-q] [-n] [--bare] [--mirror]
[-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
[--separate-git-dir <git dir>]
- [--depth <depth>] [--recursive|--recurse-submodules] [--] <repository>
+ [--depth <depth>] [--[no-]single-branch]
+ [--recursive|--recurse-submodules] [--] <repository>
[<directory>]
DESCRIPTION
@@ -179,6 +180,14 @@ objects from the source repository into a pack in the cloned repository.
with a long history, and would want to send in fixes
as patches.
+--single-branch::
+ Clone only the history leading to the tip of a single branch,
+ either specified by the `--branch` option or the primary
+ branch remote's `HEAD` points at. When creating a shallow
+ clone with the `--depth` option, this is the default, unless
+ `--no-single-branch` is given to fetch the histories near the
+ tips of all branches.
+
--recursive::
--recurse-submodules::
After the clone is created, initialize all submodules within,
diff --git a/builtin/clone.c b/builtin/clone.c
index 86db954..9dcc5fe 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -37,7 +37,7 @@ static const char * const builtin_clone_usage[] = {
NULL
};
-static int option_no_checkout, option_bare, option_mirror;
+static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
static int option_local, option_no_hardlinks, option_shared, option_recursive;
static char *option_template, *option_depth;
static char *option_origin = NULL;
@@ -48,6 +48,7 @@ static int option_verbosity;
static int option_progress;
static struct string_list option_config;
static struct string_list option_reference;
+static const char *src_ref_prefix = "refs/heads/";
static int opt_parse_reference(const struct option *opt, const char *arg, int unset)
{
@@ -92,6 +93,8 @@ static struct option builtin_clone_options[] = {
"path to git-upload-pack on the remote"),
OPT_STRING(0, "depth", &option_depth, "depth",
"create a shallow clone of that depth"),
+ OPT_BOOL(0, "single-branch", &option_single_branch,
+ "clone only one branch, HEAD or --branch"),
OPT_STRING(0, "separate-git-dir", &real_git_dir, "gitdir",
"separate git dir from working tree"),
OPT_STRING_LIST('c', "config", &option_config, "key=value",
@@ -427,8 +430,28 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
struct ref *local_refs = head;
struct ref **tail = head ? &head->next : &local_refs;
- get_fetch_map(refs, refspec, &tail, 0);
- if (!option_mirror)
+ if (option_single_branch) {
+ struct ref *remote_head = NULL;
+
+ if (!option_branch)
+ remote_head = guess_remote_head(head, refs, 0);
+ else {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, src_ref_prefix);
+ strbuf_addstr(&sb, option_branch);
+ remote_head = find_ref_by_name(refs, sb.buf);
+ strbuf_release(&sb);
+ }
+
+ if (!remote_head && option_branch)
+ warning(_("Could not find remote branch %s to clone."),
+ option_branch);
+ else
+ get_fetch_map(remote_head, refspec, &tail, 0);
+ } else
+ get_fetch_map(refs, refspec, &tail, 0);
+
+ if (!option_mirror && !option_single_branch)
get_fetch_map(refs, tag_refspec, &tail, 0);
return local_refs;
@@ -448,6 +471,21 @@ static void write_remote_refs(const struct ref *local_refs)
clear_extra_refs();
}
+static void write_followtags(const struct ref *refs, const char *msg)
+{
+ const struct ref *ref;
+ for (ref = refs; ref; ref = ref->next) {
+ if (prefixcmp(ref->name, "refs/tags/"))
+ continue;
+ if (!suffixcmp(ref->name, "^{}"))
+ continue;
+ if (!has_sha1_file(ref->old_sha1))
+ continue;
+ update_ref(msg, ref->name, ref->old_sha1,
+ NULL, 0, DIE_ON_ERR);
+ }
+}
+
static int write_one_config(const char *key, const char *value, void *data)
{
return git_config_set_multivar(key, value ? value : "true", "^$", 0);
@@ -478,7 +516,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
struct transport *transport = NULL;
- char *src_ref_prefix = "refs/heads/";
int err = 0;
struct refspec *refspec;
@@ -498,6 +535,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
+ if (option_single_branch == -1)
+ option_single_branch = option_depth ? 1 : 0;
+
if (option_mirror)
option_bare = 1;
@@ -645,6 +685,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
+ if (option_single_branch)
+ transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
transport_set_verbosity(transport, option_verbosity, option_progress);
@@ -663,6 +705,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
clear_extra_refs();
write_remote_refs(mapped_refs);
+ if (option_single_branch)
+ write_followtags(refs, reflog_msg.buf);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9bf69e9..7e85c71 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -114,8 +114,19 @@ pull_to_client 2nd "refs/heads/B" $((64*3))
pull_to_client 3rd "refs/heads/A" $((1*3))
+test_expect_success 'single branch clone' '
+ git clone --single-branch "file://$(pwd)/." singlebranch
+'
+
+test_expect_success 'single branch object count' '
+ GIT_DIR=singlebranch/.git git count-objects -v |
+ grep "^in-pack:" > count.singlebranch &&
+ echo "in-pack: 198" >expected &&
+ test_cmp expected count.singlebranch
+'
+
test_expect_success 'clone shallow' '
- git clone --depth 2 "file://$(pwd)/." shallow
+ git clone --no-single-branch --depth 2 "file://$(pwd)/." shallow
'
test_expect_success 'clone shallow object count' '
@@ -248,4 +259,63 @@ test_expect_success 'clone shallow object count' '
grep "^count: 52" count.shallow
'
+test_expect_success 'clone shallow without --no-single-branch' '
+ git clone --depth 1 "file://$(pwd)/." shallow2
+'
+
+test_expect_success 'clone shallow object count' '
+ (
+ cd shallow2 &&
+ git count-objects -v
+ ) > count.shallow2 &&
+ grep "^in-pack: 6" count.shallow2
+'
+
+test_expect_success 'clone shallow with --branch' '
+ git clone --depth 1 --branch A "file://$(pwd)/." shallow3
+'
+
+test_expect_success 'clone shallow object count' '
+ echo "in-pack: 12" > count3.expected &&
+ GIT_DIR=shallow3/.git git count-objects -v |
+ grep "^in-pack" > count3.actual &&
+ test_cmp count3.expected count3.actual
+'
+
+test_expect_success 'clone shallow with nonexistent --branch' '
+ git clone --depth 1 --branch Z "file://$(pwd)/." shallow4 &&
+ GIT_DIR=shallow4/.git git rev-parse HEAD >actual &&
+ git rev-parse HEAD >expected &&
+ test_cmp expected actual
+'
+
+test_expect_success 'clone shallow with detached HEAD' '
+ git checkout HEAD^ &&
+ git clone --depth 1 "file://$(pwd)/." shallow5 &&
+ git checkout - &&
+ GIT_DIR=shallow5/.git git rev-parse HEAD >actual &&
+ git rev-parse HEAD^ >expected &&
+ test_cmp expected actual
+'
+
+test_expect_success 'shallow clone pulling tags' '
+ git tag -a -m A TAGA1 A &&
+ git tag -a -m B TAGB1 B &&
+ git tag TAGA2 A &&
+ git tag TAGB2 B &&
+ git clone --depth 1 "file://$(pwd)/." shallow6 &&
+
+ cat >taglist.expected <<\EOF &&
+TAGB1
+TAGB2
+EOF
+ GIT_DIR=shallow6/.git git tag -l >taglist.actual &&
+ test_cmp taglist.expected taglist.actual &&
+
+ echo "in-pack: 7" > count6.expected &&
+ GIT_DIR=shallow6/.git git count-objects -v |
+ grep "^in-pack" > count6.actual &&
+ test_cmp count6.expected count6.actual
+'
+
test_done
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [PATCH 2/5] run-command: kill children on exit by default
From: Jeff King @ 2012-01-07 14:50 UTC (permalink / raw)
To: Clemens Buchacher
Cc: Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <1325936567-3136-3-git-send-email-drizzd@aon.at>
On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:
> It feels natural for a user to view git commands as monolithic
> commands with a single thread of execution. If the parent git
> command dies, it should therefore clean up its child processes as
> well. So enable the cleanup mechanism by default.
I'm not sure this is a good idea. run_command is used in ~70 places in
git, and I'm sure at least one of them is going to be unhappy (I see you
found one in credential-cache, but how many others are there). I'd
rather be conservative and leave the default the same, and then switch
over callsites that make sense.
-Peff
PS I thought this would certainly break the pager, since it should
outlast us after we finish producing output. But I think at one point
I switched the pager invocation so that the git wrapper lives and
waits until the pager dies.
^ 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