* [PATCH 02/11] Resumable clone: add prime-clone endpoints
From: Kevin Wern @ 2016-09-16 0:12 UTC (permalink / raw)
To: git
In-Reply-To: <1473984742-12516-1-git-send-email-kevin.m.wern@gmail.com>
Add logic to serve git-prime-clone to git and http clients.
Do not pass --stateless-rpc and --advertise-refs options to
prime-clone. It is inherently stateless and an 'advertisement'.
Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
---
Documentation/git-daemon.txt | 7 +++++++
Documentation/git-http-backend.txt | 7 +++++++
daemon.c | 7 +++++++
http-backend.c | 22 +++++++++++++++++-----
4 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index a69b361..853faab 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -231,6 +231,13 @@ receive-pack::
enabled by setting `daemon.receivepack` configuration item to
`true`.
+primeclone::
+ This serves 'git prime-clone' service to clients, allowing
+ 'git clone' clients to get the location of a static resource
+ to download and integrate before performing an incremental
+ fetch. It is 'false' by default, but can be enabled by setting
+ it to `true`.
+
EXAMPLES
--------
We assume the following in /etc/services::
diff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt
index 9268fb6..40be74e 100644
--- a/Documentation/git-http-backend.txt
+++ b/Documentation/git-http-backend.txt
@@ -54,6 +54,13 @@ http.receivepack::
disabled by setting this item to `false`, or enabled for all
users, including anonymous users, by setting it to `true`.
+http.primeclone::
+ This serves 'git prime-clone' service to clients, allowing
+ 'git clone' clients to get the location of a static resource
+ to download and integrate before performing an incremental
+ fetch. It is 'false' by default, but can be enabled by setting
+ it to `true`.
+
URL TRANSLATION
---------------
To determine the location of the repository on disk, 'git http-backend'
diff --git a/daemon.c b/daemon.c
index 8d45c33..c2f539c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -475,10 +475,17 @@ static int receive_pack(void)
return run_service_command(argv);
}
+static int prime_clone(void)
+{
+ static const char *argv[] = { "prime-clone", "--strict", ".", NULL };
+ return run_service_command(argv);
+}
+
static struct daemon_service daemon_service[] = {
{ "upload-archive", "uploadarch", upload_archive, 0, 1 },
{ "upload-pack", "uploadpack", upload_pack, 1, 1 },
{ "receive-pack", "receivepack", receive_pack, 0, 1 },
+ { "prime-clone", "primeclone", prime_clone, 0, 1 },
};
static void enable_service(const char *name, int ena)
diff --git a/http-backend.c b/http-backend.c
index 8870a26..9c89a10 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -27,6 +27,7 @@ struct rpc_service {
static struct rpc_service rpc_service[] = {
{ "upload-pack", "uploadpack", 1, 1 },
{ "receive-pack", "receivepack", 0, -1 },
+ { "prime-clone", "primeclone", 0, -1 },
};
static struct string_list *get_parameters(void)
@@ -450,11 +451,22 @@ static void get_info_refs(char *arg)
hdr_nocache();
if (service_name) {
- const char *argv[] = {NULL /* service name */,
- "--stateless-rpc", "--advertise-refs",
- ".", NULL};
+ struct argv_array argv;
struct rpc_service *svc = select_service(service_name);
+ argv_array_init(&argv);
+ argv_array_push(&argv, svc->name);
+
+ // prime-clone does not need --stateless-rpc and
+ // --advertise-refs options. Maybe it will in the future, but
+ // until then it seems best to do this instead of adding
+ // "dummy" options.
+ if (strcmp(svc->name, "prime-clone") != 0) {
+ argv_array_pushl(&argv, "--stateless-rpc",
+ "--advertise-refs", NULL);
+ }
+
+ argv_array_pushl(&argv, ".", NULL);
strbuf_addf(&buf, "application/x-git-%s-advertisement",
svc->name);
hdr_str(content_type, buf.buf);
@@ -463,8 +475,8 @@ static void get_info_refs(char *arg)
packet_write(1, "# service=git-%s\n", svc->name);
packet_flush(1);
- argv[0] = svc->name;
- run_service(argv, 0);
+ run_service(argv.argv, 0);
+ argv_array_clear(&argv);
} else {
select_getanyfile();
--
2.7.4
^ permalink raw reply related
* [PATCH 01/11] Resumable clone: create service git-prime-clone
From: Kevin Wern @ 2016-09-16 0:12 UTC (permalink / raw)
To: git
In-Reply-To: <1473984742-12516-1-git-send-email-kevin.m.wern@gmail.com>
Create git-prime-clone, a program to be executed on the server that
returns the location and type of static resource to download before
performing the rest of a clone.
Additionally, as this executable's location will be configurable (see:
upload-pack and receive-pack), add the program to
BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
git-prime-clone executable to gitignore, as well
Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
---
.gitignore | 1 +
Documentation/git-prime-clone.txt | 39 ++++++++++++++++++++
Makefile | 2 +
builtin.h | 1 +
builtin/prime-clone.c | 77 +++++++++++++++++++++++++++++++++++++++
git.c | 1 +
6 files changed, 121 insertions(+)
create mode 100644 Documentation/git-prime-clone.txt
create mode 100644 builtin/prime-clone.c
diff --git a/.gitignore b/.gitignore
index 5087ce1..bfea25c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,6 +106,7 @@
/git-pack-refs
/git-parse-remote
/git-patch-id
+/git-prime-clone
/git-prune
/git-prune-packed
/git-pull
diff --git a/Documentation/git-prime-clone.txt b/Documentation/git-prime-clone.txt
new file mode 100644
index 0000000..fc5917d
--- /dev/null
+++ b/Documentation/git-prime-clone.txt
@@ -0,0 +1,39 @@
+git-prime-clone(1)
+============
+
+NAME
+----
+git-prime-clone - Get the location of an alternate resource
+to fetch before clone
+
+
+SYNOPSIS
+--------
+[verse]
+'git prime-clone' [--strict] <dir>
+
+DESCRIPTION
+-----------
+
+Outputs the resource, if configured to do so. Otherwise, returns
+nothing (packet flush 0000).
+
+CONFIGURE
+---------
+
+primeclone.url::
+ The full url of the resource (e.g.
+ http://examplehost/pack-$NAME.pack).
+
+primeclone.filetype::
+ The type of the resource (e.g. pack).
+
+primeclone.enabled::
+ When 'false', git-prime-clone will return an empty response,
+ regardless of what the rest of the configuration specifies;
+ otherwise, it will return the configured response. Is 'true'
+ by default.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 24bef8d..f2564ec 100644
--- a/Makefile
+++ b/Makefile
@@ -648,6 +648,7 @@ OTHER_PROGRAMS = git$X
# what test wrappers are needed and 'install' will install, in bindir
BINDIR_PROGRAMS_NEED_X += git
BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-prime-clone
BINDIR_PROGRAMS_NEED_X += git-receive-pack
BINDIR_PROGRAMS_NEED_X += git-upload-archive
BINDIR_PROGRAMS_NEED_X += git-shell
@@ -904,6 +905,7 @@ BUILTIN_OBJS += builtin/pack-objects.o
BUILTIN_OBJS += builtin/pack-redundant.o
BUILTIN_OBJS += builtin/pack-refs.o
BUILTIN_OBJS += builtin/patch-id.o
+BUILTIN_OBJS += builtin/prime-clone.o
BUILTIN_OBJS += builtin/prune-packed.o
BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
diff --git a/builtin.h b/builtin.h
index 6b95006..c9e2254 100644
--- a/builtin.h
+++ b/builtin.h
@@ -97,6 +97,7 @@ extern int cmd_notes(int argc, const char **argv, const char *prefix);
extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);
extern int cmd_patch_id(int argc, const char **argv, const char *prefix);
+extern int cmd_prime_clone(int argc, const char **argv, const char *prefix);
extern int cmd_prune(int argc, const char **argv, const char *prefix);
extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
extern int cmd_pull(int argc, const char **argv, const char *prefix);
diff --git a/builtin/prime-clone.c b/builtin/prime-clone.c
new file mode 100644
index 0000000..ce914d3
--- /dev/null
+++ b/builtin/prime-clone.c
@@ -0,0 +1,77 @@
+#include "cache.h"
+#include "parse-options.h"
+#include "pkt-line.h"
+
+static char const * const prime_clone_usage[] = {
+ N_("git prime-clone [--strict] <dir>"),
+ NULL
+};
+
+static unsigned int enabled = 1;
+static const char *url = NULL, *filetype = NULL;
+static int strict;
+
+static struct option prime_clone_options[] = {
+ OPT_BOOL(0, "strict", &strict, N_("Do not attempt <dir>/.git if <dir> "
+ "is not a git directory")),
+ OPT_END(),
+};
+
+static void prime_clone(void)
+{
+ if (!enabled) {
+ fprintf(stderr, _("prime-clone not enabled\n"));
+ }
+ else if (url && filetype){
+ packet_write(1, "%s %s\n", filetype, url);
+ }
+ else if (url || filetype) {
+ if (filetype)
+ fprintf(stderr, _("prime-clone not properly "
+ "configured: missing url\n"));
+ else if (url)
+ fprintf(stderr, _("prime-clone not properly "
+ "configured: missing filetype\n"));
+ }
+ packet_flush(1);
+}
+
+static int prime_clone_config(const char *var, const char *value, void *unused)
+{
+ if (!strcmp("primeclone.url",var)) {
+ return git_config_pathname(&url, var, value);
+ }
+ if (!strcmp("primeclone.enabled",var)) {
+ enabled = git_config_bool(var, value);
+ return 0;
+ }
+ if (!strcmp("primeclone.filetype",var)) {
+ return git_config_string(&filetype, var, value);
+ }
+ return git_default_config(var, value, unused);
+}
+
+int cmd_prime_clone(int argc, const char **argv, const char *prefix)
+{
+ const char *dir;
+ argc = parse_options(argc, argv, prefix, prime_clone_options,
+ prime_clone_usage, 0);
+ if (argc == 0) {
+ usage_msg_opt(_("No repository specified."), prime_clone_usage,
+ prime_clone_options);
+ }
+ else if (argc > 1) {
+ usage_msg_opt(_("Too many arguments."), prime_clone_usage,
+ prime_clone_options);
+ }
+
+ dir = argv[0];
+
+ if (!enter_repo(dir, 0)){
+ die(_("'%s' does not appear to be a git repository"), dir);
+ }
+
+ git_config(prime_clone_config, NULL);
+ prime_clone();
+ return 0;
+}
diff --git a/git.c b/git.c
index 6cc0c07..a5681fb 100644
--- a/git.c
+++ b/git.c
@@ -447,6 +447,7 @@ static struct cmd_struct commands[] = {
{ "pack-refs", cmd_pack_refs, RUN_SETUP },
{ "patch-id", cmd_patch_id },
{ "pickaxe", cmd_blame, RUN_SETUP },
+ { "prime-clone", cmd_prime_clone },
{ "prune", cmd_prune, RUN_SETUP },
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
--
2.7.4
^ permalink raw reply related
* [PATCH 00/11] Resumable clone
From: Kevin Wern @ 2016-09-16 0:12 UTC (permalink / raw)
To: git
Hey, all,
It's been a while (sent a very short patch in May), but I've
still been working on the resumable clone feature and checking up on
the mailing list for any updates. After submitting the prime-clone
service alone, I figured implementing the whole thing would be the best
way to understand the full scope of the problem (this is my first real
contribution here, and learning while working on such an involved
feature has not been easy).
This is a functional implementation handling a direct http/ftp URI to a
single, fully connected packfile (i.e. the link is a direct path to the
file, not a prefix or guess). My hope is that this acts as a bare
minimum cross-section spanning the full requirments that can expand in
width as more cases are added (.info file, split bundle, daemon
download service). This is certainly not perfect, but I think it at
least prototypes each component involved in the workflow.
This patch series is based on jc/bundle, because the logic to find the
tips of a pack's history already exists there (I call index-pack
--clone-bundle on the downloaded file, and read the file to write the
references to a temporary directory). If I need to re-implement this
logic or base it on another branch, let me know. For ease of pulling
and testing, I included the branch here:
https://github.com/kevinwern/git/tree/feature/prime-clone
Although there are a few changes internally from the last patch,
the "alternate resource" url to download is configured on the
server side in exactly the same way:
[primeclone]
url = http://location/pack-$NAME.pack
filetype = pack
The prime-clone service simply outputs the components as:
####url filetype
0000
On the client side, the transport_prime_clone and
transport_download_primer APIs are built to be more robust (i.e. read
messages without dying due to protocol errors), so that git clone can
always try them without being dependent on the capability output of
git-upload-pack. transport_download_primer is dependent on the success
of transport_prime_clone, but transport_prime_clone is always run on an
initial clone. Part of achieving this robustness involves adding
*_gentle functions to pkt_line, so that prime_clone can fail silently
without dying.
The transport_download_primer function uses a resumable download,
which is applicable to both automatic and manual resuming. Automatic
is programmatically reconnecting to the resource after being
interrupted (up to a set number of times). Manual is using a newly
taught --resume option on the command line:
git clone --resume <resumable_work_or_git_dir>
Right now, a manually resumable directory is left behind only if the
*client* is interrupted while a new junk mode, JUNK_LEAVE_RESUMABLE,
is set (right before the download). For an initial clone, if the
connection fails after automatic resuming, the client erases the
partial resources and falls through to a normal clone. However, once a
resumable directory is left behind by the program, it is NEVER
deleted/abandoned after it is continued with --resume.
I think determining when a resource is "unsalvageable" should be more
nuanced. Especially in a case where a connection is perpetually poor
and the user wishes to resume over a long period of time. The timeout
logic itself *definitely* needs more nuance than "repeat 5 times", such
as expanding wait times and using earlier successes when deciding to
try again. Right now, I think the most important part of this patch is
that these two paths (falling through after a failed download, exiting
to be manually resumed later) exist.
Off the top of my head, outstanding issues/TODOs inlcude:
- The above issue of determining when to fall through, when to
reattempt, and when to write the resumable info and exit
in git clone.
- Creating git-daemon service to download a resumable resource.
Pretty straightforward, I think, especially if
http.getanyfile already exists. This falls more under
"haven't gotten to yet" than dilemma.
- Logic for git clone to determine when a full clone would
be superior, such as when a clone is local or a reference is
given.
- Configuring prime-clone for multiple resources, in two
dimensions: (a) resources to choose from (e.g. fall back to
a second resource if the first one doesn't work) and (b)
resources to be downloaded together or in sequence (e.g.
download http://host/this, then http://host/that). Maybe
prime-clone could also handle client preferences in terms of
filetype or protocol. For this, I just have to re-read a few
discussions about the filetypes we use to see if there are
any outliers that aren't representable in this way. I think
this is another "haven't gotten to yet".
- Related to the above, seeing if there are any outlying
resource types whose process can't be modularized into:
download to location, use, clean one way if failed, clean
another way if succeeded. The "split bundle," for example,
is retrieved (download), read for the pack location (use),
and then the packfile is retrieved (download). I believe, in
this case, all of that can be considered the "download," and
then indexing/writing can be considered "use." But I'm not
sure if there are more extreme cases.
- Creating the logic to guess a packfile, and append that to a
prefix specified by the admin. Additionally, allowing the
admin to use a custom script to use their own logic to
output the URL.
- Preventing the retry wait period (currently set by using
select()) from being interrupted by other system calls.
I believe there is a setting in libcurl, but I don't want
to make any potentially large-impact changes without
discussing it first. Plus, I believe changes to http.c were
up for discussion anyway.
- Finding if there's a more elegant way to access the alternate
resource than invoking remote-helper with a url we don't care
about (the same url that will be specified later to stdin
with "download-primer").
- Finding if there is a better way to suppress index-pack's
output than creating a run-command option specifically to
suppress stdout.
- When running with ssh and a password, the credentials are
prompted for twice. I don't know if there is a way to
preserve credentials between executions. I couldn't find any
examples in git's source.
Some of these are issues I've been actively working on, but I'm
hitting a point where keeping everyone up-to-date trumps completeness.
Hopefully, the bulk of the 'learning and re-doing' is done and I can
update more frequently in smaller increments.
I will probably work on the git-daemon download service, the curl
timeout issue, and supporting other filetypes next.
Feedback is appreciated.
Kevin Wern (11):
Resumable clone: create service git-prime-clone
Resumable clone: add prime-clone endpoints
pkt-line: create gentle packet_read_line functions
Resumable clone: add prime-clone to remote-curl
Resumable clone: add output parsing to connect.c
Resumable clone: implement transport_prime_clone
Resumable clone: add resumable download to http/curl
Resumable clone: create transport_download_primer
path: add resumable marker
run command: add RUN_COMMAND_NO_STDOUT
Resumable clone: implement primer logic in git-clone
.gitignore | 1 +
Documentation/git-clone.txt | 16 +
Documentation/git-daemon.txt | 7 +
Documentation/git-http-backend.txt | 7 +
Documentation/git-prime-clone.txt | 39 +++
Makefile | 2 +
builtin.h | 1 +
builtin/clone.c | 590 +++++++++++++++++++++++++++++++------
builtin/prime-clone.c | 77 +++++
cache.h | 1 +
connect.c | 47 +++
connect.h | 10 +-
daemon.c | 7 +
git.c | 1 +
http-backend.c | 22 +-
http.c | 86 +++++-
http.h | 7 +-
path.c | 1 +
pkt-line.c | 47 ++-
pkt-line.h | 16 +
remote-curl.c | 192 +++++++++---
run-command.c | 1 +
run-command.h | 1 +
t/t9904-git-prime-clone.sh | 181 ++++++++++++
transport-helper.c | 75 ++++-
transport.c | 53 ++++
transport.h | 27 ++
27 files changed, 1361 insertions(+), 154 deletions(-)
create mode 100644 Documentation/git-prime-clone.txt
create mode 100644 builtin/prime-clone.c
create mode 100755 t/t9904-git-prime-clone.sh
--
2.7.4
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: brian m. carlson @ 2016-09-15 23:47 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Jeff King
In-Reply-To: <f7294ac5-8302-03fb-d756-81a1c029a813@web.de>
[-- Attachment #1: Type: text/plain, Size: 779 bytes --]
On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
> Replace uses of strbuf_addf() for adding strings with more lightweight
> strbuf_addstr() calls. This makes the intent clearer and avoids
> potential issues with printf format specifiers.
>
> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
> this patch covers eleven more.
>
> A semantic patch for Coccinelle is included for easier checking for
> new cases that might be introduced in the future.
I think all three of these patches look good. I'm glad to see us
getting better use out of Coccinelle.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC] extending pathspec support to submodules
From: Stefan Beller @ 2016-09-15 22:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Heiko Voigt, git@vger.kernel.org, Duy Nguyen,
Jens Lehmann
In-Reply-To: <xmqqr38klst6.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 15, 2016 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> You're right that seems like the best course of action and it already falls
>> inline with what I did with a first patch to ls-files to support submodules.
>> In that patch I did exactly as you suggest and pass in the prefix to the
>> submodule and make the child responsible for prepending the prefix to all of
>> its output. This way we can simply pass through the whole pathspec (as apposed
>> to my original idea of stripping the prefix off the pathspec prior to passing
>> it to the child...which can get complicated with wild characters) to the
>> childprocess and when checking if a file matches the pathspec we can check if
>> the prefix + file path matches.
>
> That's brilliant. A few observations.
>
> * With that change to tell the command that is spawned in a
> submodule directory where the submodule repository is in the
> context of the top-level superproject _and_ require it to take a
> pathspec as relative to the top-level superproject, you no longer
> worry about having to find where to cut the pathspec given at the
> top-level to adjust it for the submodule's context. That may
> simplify things.
I wonder how this plays together with the prefix in the superproject, e.g.
cd super/unrelated-path
# when invoking a git command the internal prefix is "unrelated-path/"
git ls-files ../submodule-*
# a submodule in submodule-A would be run in submodule-A
# with a superproject prefix of super/ ? but additionally we nned
to know we're
# not at the root of the superproject.
> So we may have to rethink what this option name should be. "You
> are running in a repository that is used as a submodule in a
> larger context, which has the submodule at this path" is what the
> option tells the command; if any existing command already has
> such an option, we should use it. If we are inventing one,
> perhaps "--submodule-path" (I didn't check if there are existing
> options that sound similar to it and mean completely different
> things, in which case that name is not usable)?
Would it make sense to add the '--submodule-path' to a more generic
part of the code? It's not just ls-files/grep that have to solve exactly this
problem. Up to now we just did not go for those commands, though.
Thanks
^ permalink raw reply
* Re: [RFC] extending pathspec support to submodules
From: Junio C Hamano @ 2016-09-15 22:08 UTC (permalink / raw)
To: Brandon Williams; +Cc: Heiko Voigt, git, pclouds, Jens Lehmann, Stefan Beller
In-Reply-To: <CAKoko1rtEydwbWoEq9MBW41qqa10Bm+x0d6zS+Bptk51RjMOMA@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
> You're right that seems like the best course of action and it already falls
> inline with what I did with a first patch to ls-files to support submodules.
> In that patch I did exactly as you suggest and pass in the prefix to the
> submodule and make the child responsible for prepending the prefix to all of
> its output. This way we can simply pass through the whole pathspec (as apposed
> to my original idea of stripping the prefix off the pathspec prior to passing
> it to the child...which can get complicated with wild characters) to the
> childprocess and when checking if a file matches the pathspec we can check if
> the prefix + file path matches.
That's brilliant. A few observations.
* With that change to tell the command that is spawned in a
submodule directory where the submodule repository is in the
context of the top-level superproject _and_ require it to take a
pathspec as relative to the top-level superproject, you no longer
worry about having to find where to cut the pathspec given at the
top-level to adjust it for the submodule's context. That may
simplify things.
* Your program that runs in the top-level superproject still needs
to be able to say "this pathspec from the top cannot possibly
match anything in the submodule, so let's not even bother
descending into it".
* Earlier while reviewing "ls-files" recursion, I suggested (and
you took) --output-path-prefix as the option name, because it was
meant to be "when you output any path, prefix this string". But
the suggested name is suboptimal, as it is no longer an option
that is only about "output". A command that runs in a submodule
would:
- enumerate paths in the context of the submodule repository,
- prepend the "prefix" to these paths,
- filter by applying the full-tree pathspec, and
- work on the surviving paths after filtering.
When the last step, "work on", involves just "printing", the
whole path (with "prefix") is sent to the output. If it involves
some operation relative to the submodule repository (e.g. seeing
if it is in the index), the "prefix" may have to be stripped
while the operation is carried out.
So we may have to rethink what this option name should be. "You
are running in a repository that is used as a submodule in a
larger context, which has the submodule at this path" is what the
option tells the command; if any existing command already has
such an option, we should use it. If we are inventing one,
perhaps "--submodule-path" (I didn't check if there are existing
options that sound similar to it and mean completely different
things, in which case that name is not usable)?
Thanks.
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: Junio C Hamano @ 2016-09-15 21:39 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Git List, brian m. carlson
In-Reply-To: <67756074-836f-2238-37c3-0d186325bd00@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 15.09.2016 um 22:01 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Take this for example:
>>>
>>> - strbuf_addf(&o->obuf, _("(bad commit)\n"));
>>> + strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>>>
>>> If there's a language that uses percent signs instead of parens or as
>>> regular letters, then they need to be escaped in the translated string
>>> before, but not after the patch. As I wrote: silly.
>>
>> Ahh, OK, so "This use of addf only has format part and nothing else,
>> hence the format part can be taken as-is" which is the Coccinelle rule
>> used to produce this patch is incomplete and always needs manual
>> inspection, in case the format part wanted to give a literal % in
>> the output. E.g. it is a bug to convert this
>>
>> strbuf_addf(&buf, _("this is 100%% wrong!"));
>>
>> to
>>
>> strbuf_addstr(&buf, _("this is 100%% wrong!"));
>
> Right. Such strings seem to be quite rare in practice, though.
>
>> Thanks for clarification. Perhaps the strbuf.cocci rule file can
>> have some comment to warn the person who builds *.patch file to look
>> for % in E2, or something?
>
> Something like this?
Yup, with something like that I would understdood where that
puzzling question came from.
Thanks.
>
> ---
> contrib/coccinelle/strbuf.cocci | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index 7932d48..3f535ca 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -1,3 +1,5 @@
> +// Careful, this is not fully equivalent: "%" is no longer treated
> +// specially. Check for "%%", "%m" etc. in the format string (E2).
> @@
> expression E1, E2;
> @@
^ permalink raw reply
* Re: [PATCH 2/2] SQUASH??? Undecided
From: Brandon Williams @ 2016-09-15 21:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79kaCVZ-Z+XSYWK6YtkT8L=pDrDQE-pAyseHNf5w2NO5XMw@mail.gmail.com>
Yeah if that is the convention then I have no problem with the change.
-Brandon
On Thu, Sep 15, 2016 at 2:12 PM, Stefan Beller <sbeller@google.com> wrote:
> + cc Brandon
>
> On Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> If we were to follow the convention to leave an optional string
>> variable to NULL, we'd need to do this on top. I am not sure if it
>> is a good change, though.
>
> I think it is a good change.
>
> Thanks,
> Stefan
>
>> ---
>> builtin/ls-files.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index 6e78c71..687e475 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -29,7 +29,7 @@ static int show_valid_bit;
>> static int line_terminator = '\n';
>> static int debug_mode;
>> static int show_eol;
>> -static const char *output_path_prefix = "";
>> +static const char *output_path_prefix;
>> static int recurse_submodules;
>>
>> static const char *prefix;
>> @@ -78,7 +78,7 @@ static void write_name(const char *name)
>> * churn.
>> */
>> static struct strbuf full_name = STRBUF_INIT;
>> - if (*output_path_prefix) {
>> + if (output_path_prefix && *output_path_prefix) {
>> strbuf_reset(&full_name);
>> strbuf_addstr(&full_name, output_path_prefix);
>> strbuf_addstr(&full_name, name);
>> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
>> argv_array_push(&cp.args, "ls-files");
>> argv_array_push(&cp.args, "--recurse-submodules");
>> argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
>> - output_path_prefix, ce->name);
>> + output_path_prefix ? output_path_prefix : "",
>> + ce->name);
>> cp.git_cmd = 1;
>> cp.dir = ce->name;
>> status = run_command(&cp);
>> --
>> 2.10.0-458-g97b4043
>>
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: René Scharfe @ 2016-09-15 21:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git List, brian m. carlson
In-Reply-To: <xmqq7fadnd9s.fsf@gitster.mtv.corp.google.com>
Am 15.09.2016 um 22:01 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Take this for example:
>>
>> - strbuf_addf(&o->obuf, _("(bad commit)\n"));
>> + strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>>
>> If there's a language that uses percent signs instead of parens or as
>> regular letters, then they need to be escaped in the translated string
>> before, but not after the patch. As I wrote: silly.
>
> Ahh, OK, so "This use of addf only has format part and nothing else,
> hence the format part can be taken as-is" which is the Coccinelle rule
> used to produce this patch is incomplete and always needs manual
> inspection, in case the format part wanted to give a literal % in
> the output. E.g. it is a bug to convert this
>
> strbuf_addf(&buf, _("this is 100%% wrong!"));
>
> to
>
> strbuf_addstr(&buf, _("this is 100%% wrong!"));
Right. Such strings seem to be quite rare in practice, though.
> Thanks for clarification. Perhaps the strbuf.cocci rule file can
> have some comment to warn the person who builds *.patch file to look
> for % in E2, or something?
Something like this?
---
contrib/coccinelle/strbuf.cocci | 2 ++
1 file changed, 2 insertions(+)
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 7932d48..3f535ca 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -1,3 +1,5 @@
+// Careful, this is not fully equivalent: "%" is no longer treated
+// specially. Check for "%%", "%m" etc. in the format string (E2).
@@
expression E1, E2;
@@
--
2.10.0
^ permalink raw reply related
* [PATCH v2 1/1] git-p4: Add --checkpoint-period option to sync/clone
From: Ori Rawlings @ 2016-09-15 21:17 UTC (permalink / raw)
To: git; +Cc: Vitor Antunes, Lars Schneider, Luke Diamand, Pete Wyckoff,
Ori Rawlings
In-Reply-To: <cover.9c54bbdd9f054215b5432c4ba3081110e2e91724.1473973732.git-series.orirawlings@gmail.com>
Importing a long history from Perforce into git using the git-p4 tool
can be especially challenging. The `git p4 clone` operation is based
on an all-or-nothing transactionality guarantee. Under real-world
conditions like network unreliability or a busy Perforce server,
`git p4 clone` and `git p4 sync` operations can easily fail, forcing a
user to restart the import process from the beginning. The longer the
history being imported, the more likely a fault occurs during the
process. Long enough imports thus become statistically unlikely to ever
succeed.
The underlying git fast-import protocol supports an explicit checkpoint
command. The idea here is to optionally allow the user to force an
explicit checkpoint every <x> seconds. If the sync/clone operation fails
branches are left updated at the appropriate commit available during the
latest checkpoint. This allows a user to resume importing Perforce
history while only having to repeat at most approximately <x> seconds
worth of import activity.
Signed-off-by: Ori Rawlings <orirawlings@gmail.com>
---
Documentation/git-p4.txt | 12 ++++++-
git-p4.py | 7 ++++-
t/t9830-git-p4-checkpoint-period.sh | 59 ++++++++++++++++++++++++++++++-
3 files changed, 78 insertions(+), 0 deletions(-)
create mode 100755 t/t9830-git-p4-checkpoint-period.sh
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index c83aaf3..e48ed6d 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -252,6 +252,18 @@ Git repository:
Use a client spec to find the list of interesting files in p4.
See the "CLIENT SPEC" section below.
+--checkpoint-period <n>::
+ Issue explicit 'checkpoint' commands to the underlying
+ linkgit:git-fast-import[1] approximately every 'n' seconds. If
+ syncing or cloning from the Perforce server is interrupted, the
+ process can be resumed from the most recent checkpoint with a
+ new 'sync' invocation. This is useful in the situations where a
+ large amount of changes are being imported over an unreliable
+ network connection. Explicit checkpoints can take up to several
+ minutes each, so a suitable value for the checkpoint period is
+ approximately 1200 seconds. By default, no explicit checkpoints
+ are performed.
+
-/ <path>::
Exclude selected depot paths when cloning or syncing.
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..4c84871 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2244,6 +2244,7 @@ class P4Sync(Command, P4UserMap):
optparse.make_option("-/", dest="cloneExclude",
action="append", type="string",
help="exclude depot path"),
+ optparse.make_option("--checkpoint-period", dest="checkpointPeriod", type="int", help="Period in seconds between explict git fast-import checkpoints (by default, no explicit checkpoints are performed)"),
]
self.description = """Imports from Perforce into a git repository.\n
example:
@@ -2276,6 +2277,7 @@ class P4Sync(Command, P4UserMap):
self.tempBranches = []
self.tempBranchLocation = "refs/git-p4-tmp"
self.largeFileSystem = None
+ self.checkpointPeriod = None
if gitConfig('git-p4.largeFileSystem'):
largeFileSystemConstructor = globals()[gitConfig('git-p4.largeFileSystem')]
@@ -3031,6 +3033,7 @@ class P4Sync(Command, P4UserMap):
def importChanges(self, changes):
cnt = 1
+ self.lastCheckpointTime = time.time()
for change in changes:
description = p4_describe(change)
self.updateOptionDict(description)
@@ -3107,6 +3110,10 @@ class P4Sync(Command, P4UserMap):
self.initialParent)
# only needed once, to connect to the previous commit
self.initialParent = ""
+
+ if self.checkpointPeriod >= 0 and time.time() - self.lastCheckpointTime >= self.checkpointPeriod:
+ self.checkpoint()
+ self.lastCheckpointTime = time.time()
except IOError:
print self.gitError.read()
sys.exit(1)
diff --git a/t/t9830-git-p4-checkpoint-period.sh b/t/t9830-git-p4-checkpoint-period.sh
new file mode 100755
index 0000000..6ba4914
--- /dev/null
+++ b/t/t9830-git-p4-checkpoint-period.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='git p4 checkpoint-period tests'
+
+. ./lib-git-p4.sh
+
+p4_submit_each () {
+ for file in $@
+ do
+ echo $file > "$file" &&
+ p4 add "$file" &&
+ p4 submit -d "$file"
+ done
+}
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'no explicit checkpoints' '
+ cd "$cli" &&
+ p4_submit_each file1 file2 file3 &&
+ git p4 clone --dest="$git" //depot@all &&
+ test_when_finished cleanup_git &&
+ (
+ git -C "$git" reflog refs/remotes/p4/master >lines &&
+ test_line_count = 1 lines &&
+ p4_submit_each file4 file5 file6 &&
+ git -C "$git" p4 sync &&
+ git -C "$git" reflog refs/remotes/p4/master >lines &&
+ test_line_count = 2 lines
+ )
+'
+
+test_expect_success 'restart p4d' '
+ kill_p4d &&
+ start_p4d
+'
+
+test_expect_success 'checkpoint every 0 seconds, i.e. every commit' '
+ cd "$cli" &&
+ p4_submit_each file1 file2 file3 &&
+ git p4 clone --dest="$git" --checkpoint-period 0 //depot@all &&
+ test_when_finished cleanup_git &&
+ (
+ git -C "$git" reflog refs/remotes/p4/master >lines &&
+ test_line_count = 3 lines &&
+ p4_submit_each file4 file5 file6 &&
+ git -C "$git" p4 sync --checkpoint-period 0 &&
+ git -C "$git" reflog refs/remotes/p4/master >lines &&
+ test_line_count = 6 lines
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
git-series 0.8.10
^ permalink raw reply related
* [PATCH v2 0/1] git-p4: Add --checkpoint-period option to sync/clone
From: Ori Rawlings @ 2016-09-15 21:17 UTC (permalink / raw)
To: git; +Cc: Vitor Antunes, Lars Schneider, Luke Diamand, Pete Wyckoff,
Ori Rawlings
In-Reply-To: <1473717733-65682-1-git-send-email-orirawlings@gmail.com>
Importing a long history from Perforce into git using the git-p4 tool
can be especially challenging. The `git p4 clone` operation is based
on an all-or-nothing transactionality guarantee. Under real-world
conditions like network unreliability or a busy Perforce server,
`git p4 clone` and `git p4 sync` operations can easily fail, forcing a
user to restart the import process from the beginning. The longer the
history being imported, the more likely a fault occurs during the
process. Long enough imports thus become statistically unlikely to ever
succeed.
My idea was to leverage the checkpoint feature of git fast-import.
I've included a patch which exposes a new option to the sync/clone
commands in the git-p4 tool. The option enables explict checkpoints on
a periodic basis (approximately every x seconds).
If the sync/clone command fails during processing of Perforce changes,
the user can craft a new git p4 sync command that will identify
changes that have already been imported and proceed with importing
only changes more recent than the last successful checkpoint.
In v2 of this patch series I've added some basic test scenarios,
documentation, and did some minor clean up of the implementation
based on feedback on v1.
Ori Rawlings (1):
git-p4: Add --checkpoint-period option to sync/clone
Documentation/git-p4.txt | 12 ++++++-
git-p4.py | 7 ++++-
t/t9830-git-p4-checkpoint-period.sh | 59 ++++++++++++++++++++++++++++++-
3 files changed, 78 insertions(+), 0 deletions(-)
create mode 100755 t/t9830-git-p4-checkpoint-period.sh
--
git-series 0.8.10
^ permalink raw reply
* Re: [PATCH 2/2] SQUASH??? Undecided
From: Stefan Beller @ 2016-09-15 21:12 UTC (permalink / raw)
To: Junio C Hamano, Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <20160915205109.12240-3-gitster@pobox.com>
+ cc Brandon
On Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If we were to follow the convention to leave an optional string
> variable to NULL, we'd need to do this on top. I am not sure if it
> is a good change, though.
I think it is a good change.
Thanks,
Stefan
> ---
> builtin/ls-files.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 6e78c71..687e475 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -29,7 +29,7 @@ static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> static int show_eol;
> -static const char *output_path_prefix = "";
> +static const char *output_path_prefix;
> static int recurse_submodules;
>
> static const char *prefix;
> @@ -78,7 +78,7 @@ static void write_name(const char *name)
> * churn.
> */
> static struct strbuf full_name = STRBUF_INIT;
> - if (*output_path_prefix) {
> + if (output_path_prefix && *output_path_prefix) {
> strbuf_reset(&full_name);
> strbuf_addstr(&full_name, output_path_prefix);
> strbuf_addstr(&full_name, name);
> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
> argv_array_push(&cp.args, "ls-files");
> argv_array_push(&cp.args, "--recurse-submodules");
> argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
> - output_path_prefix, ce->name);
> + output_path_prefix ? output_path_prefix : "",
> + ce->name);
> cp.git_cmd = 1;
> cp.dir = ce->name;
> status = run_command(&cp);
> --
> 2.10.0-458-g97b4043
>
^ permalink raw reply
* Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
From: Junio C Hamano @ 2016-09-15 21:08 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160915121044.GA96648@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> struct child_process cp = CHILD_PROCESS_INIT;
> - const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
> +
> + argv_array_push(&cp.args, "rev-list");
> + sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);
> + argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
> +
> struct strbuf buf = STRBUF_INIT;
> int needs_pushing = 0;
These two become decl-after-stmt; move your new lines a bit lower,
perhaps?
> - argv[1] = sha1_to_hex(sha1);
> - cp.argv = argv;
> prepare_submodule_repo_env(&cp.env_array);
By the way, with the two new patches, 'pu' seems to start failing
some tests, e.g. 5533 5404 5405.
^ permalink raw reply
* [PATCH 1/2] SQUASH???
From: Junio C Hamano @ 2016-09-15 20:51 UTC (permalink / raw)
To: git
In-Reply-To: <20160915205109.12240-1-gitster@pobox.com>
---
builtin/ls-files.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0bce00..6e78c71 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -78,7 +78,7 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (output_path_prefix != '\0') {
+ if (*output_path_prefix) {
strbuf_reset(&full_name);
strbuf_addstr(&full_name, output_path_prefix);
strbuf_addstr(&full_name, name);
--
2.10.0-458-g97b4043
^ permalink raw reply related
* [PATCH 2/2] SQUASH??? Undecided
From: Junio C Hamano @ 2016-09-15 20:51 UTC (permalink / raw)
To: git
In-Reply-To: <20160915205109.12240-1-gitster@pobox.com>
If we were to follow the convention to leave an optional string
variable to NULL, we'd need to do this on top. I am not sure if it
is a good change, though.
---
builtin/ls-files.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6e78c71..687e475 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
-static const char *output_path_prefix = "";
+static const char *output_path_prefix;
static int recurse_submodules;
static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (*output_path_prefix) {
+ if (output_path_prefix && *output_path_prefix) {
strbuf_reset(&full_name);
strbuf_addstr(&full_name, output_path_prefix);
strbuf_addstr(&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
- output_path_prefix, ce->name);
+ output_path_prefix ? output_path_prefix : "",
+ ce->name);
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
--
2.10.0-458-g97b4043
^ permalink raw reply related
* Re: [PATCH v2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-15 20:51 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqzinbvk15.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Thanks, will queue with a minimum fix.
So here are two squashable patches, one is the "minimum" one, the
other is a bit more invasive one to use "a pointer to an optional
setting is set to NULL" convention. I am undecided, and I'll stay
to be without further comments from others, on the latter one.
I understand that many internal changes in your work environment
titles their changes like "DOing X", but our convention around here
is to label them "DO X", as if you are giving an order to somebody
else, either telling the codebase "to be like so", or telling the
patch-monkey maintainer "to make it so". So I'd retitle it
ls-files: optionally recurse into submodules
or something like that. It is an added advantage of being a lot
more descriptive than "adding support", which does not say what kind
of support it is adding.
^ permalink raw reply
* Re: [PATCH v2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-15 20:58 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <xmqqzinbvk15.fsf@gitster.mtv.corp.google.com>
If we were to follow the convention to leave an optional string
variable to NULL, we'd need to do this on top. I am not sure if it
is a good change, though.
---
builtin/ls-files.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6e78c71..687e475 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
-static const char *output_path_prefix = "";
+static const char *output_path_prefix;
static int recurse_submodules;
static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (*output_path_prefix) {
+ if (output_path_prefix && *output_path_prefix) {
strbuf_reset(&full_name);
strbuf_addstr(&full_name, output_path_prefix);
strbuf_addstr(&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
- output_path_prefix, ce->name);
+ output_path_prefix ? output_path_prefix : "",
+ ce->name);
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
--
2.10.0-458-g97b4043
^ permalink raw reply related
* Re: [PATCH v2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-15 20:58 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <xmqqzinbvk15.fsf@gitster.mtv.corp.google.com>
Here is an absolute mininum fix ;-)
builtin/ls-files.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0bce00..6e78c71 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -78,7 +78,7 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (output_path_prefix != '\0') {
+ if (*output_path_prefix) {
strbuf_reset(&full_name);
strbuf_addstr(&full_name, output_path_prefix);
strbuf_addstr(&full_name, name);
--
2.10.0-458-g97b4043
^ permalink raw reply related
* Re: [PATCH v2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-15 20:57 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <xmqqzinbvk15.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Thanks, will queue with a minimum fix.
So here are two squashable patches, one is the "minimum" one, the
other is a bit more invasive one to use "a pointer to an optional
setting is set to NULL" convention. I am undecided, and I'll stay
to be without further comments from others, on the latter one.
I understand that many internal changes in your work environment are
titled like "DOing X", but our convention around here is to label
them "DO X", as if you are giving an order to somebody else, either
telling the codebase "to be like so", or telling the patch-monkey
maintainer "to make it so". So I'd retitle it
ls-files: optionally recurse into submodules
or something like that. It is an added advantage of being a lot
more descriptive than "adding support", which does not say what kind
of support it is adding.
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Junio C Hamano @ 2016-09-15 20:33 UTC (permalink / raw)
To: Lars Schneider
Cc: Jeff King, Git Mailing List, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <744FA3D5-888A-4032-90C4-6BFC7D5D4010@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> So the "right" pattern is either:
>>
>> 1. Return -1 and the caller is responsible for telling the user.
>>
... which is valid only if there aren't different kinds of errors
that all return -1; with "return error(...)" with different
messages, the users can tell what kind of error they got (while the
caller may just do the same abort-procedure no matter what kind of
error it got), but if all of them are replaced with "return -1", the
caller cannot produce different error messages to tell the users.
>> 2. Return -1 and stuff the error into an error strbuf, so it can be
>> passed up the call chain easily (and callers do not have to come up
>> with their own wording).
... and this would become one of the viable options (the other is to
define your own error code so that the caller can tell what error it
got).
>> But if all current callers would just call error() themselves anyway,
>> then it's OK to punt on this and let somebody else handle it later if
>> they add a new caller who wants different behavior (and that is what
>> Junio was saying above, I think).
Yes. Just keeping it noisy until somebody wants a quiet-and-gentle
version is probably the best course to take.
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Junio C Hamano @ 2016-09-15 20:24 UTC (permalink / raw)
To: Lars Schneider
Cc: GIT Mailing-list, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <928655D2-312B-4805-99DB-E73448232B9A@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 13 Sep 2016, at 17:22, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> larsxschneider@gmail.com writes:
>>
>>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
>>> ...
>>> +packet_write( "clean=true\n" );
>>> +packet_write( "smudge=true\n" );
>>
>> These extra SP around the contents inside () pair look unfamiliar
>> and somewhat strange to me, but as long as they are consistently
>> done (and I think you are mostly being consistent), it is OK.
>
> Ups. I forgot to run PerlTidy here. I run PerlTidy with the flag
> "-pbp" (= Perl Best Practices). This seems to add no extra SP for
> functions with one parameter (e.g. `foo("bar")`) and extra SP
> for functions with multiple parameter (e.g. `foo( "bar", 1 )`).
> Is this still OK?
Your choice. I already said I do not care too much either way as
long as you are consistent.
If you prefer PerlTidy's PBP output over what you wrote, and if you
are resending the patch anyway, then why not? ;-)
> Does anyone have a "Git PerlTidy configuration"?
Not me.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Lars Schneider @ 2016-09-15 20:19 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Git Mailing List, sbeller, Johannes.Schindelin,
jnareb, mlbright, tboegi, jacob.keller
In-Reply-To: <20160915194443.x7zvkkryvworqcxt@sigill.intra.peff.net>
> On 15 Sep 2016, at 21:44, Jeff King <peff@peff.net> wrote:
>
> On Thu, Sep 15, 2016 at 05:42:58PM +0100, Lars Schneider wrote:
>
>>>>>> +int packet_flush_gently(int fd)
>>>>>> +{
>>>>>> + packet_trace("0000", 4, 1);
>>>>>> + if (write_in_full(fd, "0000", 4) == 4)
>>>>>> + return 0;
>>>>>> + error("flush packet write failed");
>>>>>> + return -1;
>> [...]
>>>>> I suspect that it is a strong sign that the caller wants to be in
>>>>> control of when and what error message is produced; otherwise it
>>>>> wouldn't be calling the _gently() variant, no?
>>>>
>>>> Agreed!
>>>
>>> I am also OK with the current form, too. Those who need to enhance
>>> it to packet_flush_gently(int fd, int quiet) can come later.
>>
>> "caller wants to be in control [...] otherwise it wouldn't be calling
>> the _gently() variant" convinced me. I would like to change it like
>> this:
>>
>> trace_printf_key(&trace_packet, "flush packet write failed");
>> return -1;
>>
>> Objections?
>
> I'm not sure that a trace makes sense, because it means that 99% of the
> time we are silent. AFAICT, the question is not "sometimes the user
> needs to see an error and sometimes not, and they should decide before
> starting the program". It is "sometimes the caller will report the error
> to the user as appropriate, and sometimes we need to do so". And only
> the calling code knows which is which.
>
> So the "right" pattern is either:
>
> 1. Return -1 and the caller is responsible for telling the user.
>
> or
>
> 2. Return -1 and stuff the error into an error strbuf, so it can be
> passed up the call chain easily (and callers do not have to come up
> with their own wording).
>
> But if all current callers would just call error() themselves anyway,
> then it's OK to punt on this and let somebody else handle it later if
> they add a new caller who wants different behavior (and that is what
> Junio was saying above, I think).
OK. I'll go with 1. then.
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v3 13/14] i18n: show-branch: mark plural strings for translation
From: Junio C Hamano @ 2016-09-15 20:19 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1473951548-31733-13-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Mark plural string for translation using Q_().
>
> Although we already know that the plural sentence is always used in the
> English source, other languages have complex plural rules they must
> comply according to the value of MAX_REVS.
Nicely explained. Thanks.
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-09-15 20:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: GIT Mailing-list, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <xmqq8tuvx1sz.fsf@gitster.mtv.corp.google.com>
> On 13 Sep 2016, at 17:22, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
>> ...
>> +sub packet_read {
>> + my $buffer;
>> + my $bytes_read = read STDIN, $buffer, 4;
>> + if ( $bytes_read == 0 ) {
>> +
>> + # EOF - Git stopped talking to us!
>> + exit();
>> +...
>> +packet_write( "clean=true\n" );
>> +packet_write( "smudge=true\n" );
>> +packet_flush();
>> +
>> +while (1) {
>
> These extra SP around the contents inside () pair look unfamiliar
> and somewhat strange to me, but as long as they are consistently
> done (and I think you are mostly being consistent), it is OK.
Ups. I forgot to run PerlTidy here. I run PerlTidy with the flag
"-pbp" (= Perl Best Practices). This seems to add no extra SP for
functions with one parameter (e.g. `foo("bar")`) and extra SP
for functions with multiple parameter (e.g. `foo( "bar", 1 )`).
Is this still OK?
Does anyone have a "Git PerlTidy configuration"?
>
>> +#define CAP_CLEAN (1u<<0)
>> +#define CAP_SMUDGE (1u<<1)
>
> As these are meant to be usable together, i.e. bits in a single flag
> word, they are of type "unsigned int", which makes perfect sense.
>
> Make sure your variables and fields that store them are of the same
> type. I think I saw "int' used to pass them in at least one place.
Fixed!
>> +static int apply_filter(const char *path, const char *src, size_t len,
>> + int fd, struct strbuf *dst, struct convert_driver *drv,
>> + const int wanted_capability)
>> +{
>> + const char* cmd = NULL;
>
> "const char *cmd = NULL;" of course.
Fixed!
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 11c37fb..f6798f8 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -10,6 +10,7 @@
>> #include "attr.h"
>> #include "split-index.h"
>> #include "dir.h"
>> +#include "convert.h"
>>
>> /*
>> * Error messages expected by scripts out of plumbing commands such as
>
> Why? The resulting file seems to compile without this addition.
Of course. That shouldn't have been part of this commit.
Thank you,
Lars
^ permalink raw reply
* Re: [PATCH v3 04/14] i18n: blame: mark error messages for translation
From: Junio C Hamano @ 2016-09-15 20:14 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1473951548-31733-4-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> @@ -2790,7 +2790,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> else {
> o = get_origin(&sb, sb.final, path);
> if (fill_blob_sha1_and_mode(o))
> - die("no such path %s in %s", path, final_commit_name);
> + die(_("no such path %s in %s"), path, final_commit_name);
This was missing in the earlier round, which is good to make it translated.
> - die("file %s has only %lu lines", path, lno);
> + die(Q_("file %s has only %lu line",
> + "file %s has only %lu lines",
> + lno), path, lno);
Looks good here, too. I would have moved "lno)," at the beginning
of the third line to the end of the second line to make it easier to
read, but this is OK.
^ 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