Re: [PATCH v2] fetch-pack: fix object_id of exact sha1
Hi Gabriel, On Sun, 28 Feb 2016, Gabriel Souza Franco wrote: > Not the cleanest conditional I've ever written, but it should handle > all cases correctly. It could be much worse: > + if (get_oid_hex(name, &oid) || > + (name[GIT_SHA1_HEXSZ] != ' ' && > + name[GIT_SHA1_HEXSZ] != '\0')) I know developers who would write this as if (get_oid_hex(name, &oid) || (name[GIT_SHA1_HEXSZ] & ' ')) and not even begin to realize that this is a problem. So I'd say your conditional is good. Having said that, this *might* be a good opportunity to imitate the skip_prefix() function. If there are enough similar code constructs, we could simplify all of them by introducing the function skip_oid_hex(const char *str, struct object_id *oid, const char **out) that returns 1 if and only if an oid was parsed, and stores the pointer after the oid in "out" (skipping an additional space if there is one)? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()
Although I was not sure [4], I decided to roll out v4, in the hope that the next
reviewers will profit by the more polished commit messages and order.
This series deals with strbuf_getline_lf() in certain codepaths:
Those, where the input that is read, is/was trimmed before doing anything that
could possibly expect a CR character. Those places can be assumed to be "text"
input, where a CR never would be a meaningful control character.
The purpose of this series is to document these places to have this property,
by using strbuf_getline() instead of strbuf_getline_lf(). Also in some
codepaths,
the CR could be a leftover of an editor and is thus removed.
Every codepath was examined, if after the change it is still necessary to have
trimming or if the additional CRLF-removal suffices.
The series is an idea out of [1], where Junio proposed to replace the calls
to strbuf_getline_lf() because it 'would [be] a good way to document them as
dealing with "text"'.
Changes since v3 [3] (the changes to single patches are indicated below):
* Commit messages refined
* Order of patch 4 and 5 in v2 was switched.
The interdiff only removes an empty line (I noticed, when changing the order of
commits, that the splitting operation had no newline before this whole series,
so I left it that way):
diff --git a/builtin/notes.c b/builtin/notes.c
index 660c0b7..715fade 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -296,7 +296,6 @@ static int notes_copy_from_stdin(int force, const char
*rewrite_cmd)
int err;
string_list_split(&split, buf.buf, ' ', -1);
-
if (split.nr != 2)
die(_("Malformed input line: '%s'."), buf.buf);
if (get_sha1(split.items[0].string, from_obj))
-Moritz
[1], idea: http://thread.gmane.org/gmane.comp.version-control.git/284104
[2], v2:
http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865
[3], v3:
http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287747
[4] http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287760
Moritz Neeb (7):
quote: remove leading space in sq_dequote_step -- as in v2
bisect: read bisect paths with strbuf_getline() -- refined commit message
clean: read user input with strbuf_getline() -- simplified commit message
notes copy --stdin: read lines with strbuf_getline() -- switched with below
notes copy --stdin: split lines with string_list_split() -- switched with
above
remote: read $GIT_DIR/branches/* with strbuf_getline() -- as in v3
wt-status: read rebase todolist with strbuf_getline() -- as in v2
bisect.c| 5 ++---
builtin/clean.c | 6 +++---
builtin/notes.c | 22 ++
quote.c | 2 ++
remote.c| 2 +-
wt-status.c | 3 +--
6 files changed, 19 insertions(+), 21 deletions(-)
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/7] quote: remove leading space in sq_dequote_step
Because sq_quote_argv adds a leading space (which is expected in trace.c),
sq_dequote_step should remove this space again, such that the operations
of quoting and dequoting are inverse of each other.
This patch is preparing the way to remove some excessive trimming
operation in bisect in the following commit.
Signed-off-by: Moritz Neeb
---
quote.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/quote.c b/quote.c
index fe884d2..2714f27 100644
--- a/quote.c
+++ b/quote.c
@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
char *src = arg;
char c;
+ if (*src == ' ')
+ src++;
if (*src != '\'')
return NULL;
for (;;) {
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/7] clean: read user input with strbuf_getline()
The inputs that are read are all answers that are given by the user
when interacting with git on the commandline. As these answers are
not supposed to contain a meaningful CR it is safe to
replace strbuf_getline_lf() by strbuf_getline().
After being read, the input is trimmed. This leads to accepting user
input with spaces, e.g. " y ", as a valid answer in the interactive
cleaning process.
Although trimming would not be required anymore to remove a potential CR,
we don't want to change the existing behavior with this patch.
Thus, the trimming is kept in place.
Signed-off-by: Moritz Neeb
---
builtin/clean.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..5b17a31 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct
menu_stuff *stuff)
clean_get_color(CLEAN_COLOR_RESET));
}
- if (strbuf_getline_lf(&choice, stdin) != EOF) {
+ if (strbuf_getline(&choice, stdin) != EOF) {
strbuf_trim(&choice);
} else {
eof = 1;
@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)
clean_print_color(CLEAN_COLOR_PROMPT);
printf(_("Input ignore patterns>> "));
clean_print_color(CLEAN_COLOR_RESET);
- if (strbuf_getline_lf(&confirm, stdin) != EOF)
+ if (strbuf_getline(&confirm, stdin) != EOF)
strbuf_trim(&confirm);
else
putchar('\n');
@@ -750,7 +750,7 @@ static int ask_each_cmd(void)
qname = quote_path_relative(item->string, NULL, &buf);
/* TRANSLATORS: Make sure to keep [y/N] as is */
printf(_("Remove %s [y/N]? "), qname);
- if (strbuf_getline_lf(&confirm, stdin) != EOF) {
+ if (strbuf_getline(&confirm, stdin) != EOF) {
strbuf_trim(&confirm);
} else {
putchar('\n');
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 7/7] wt-status: read rebase todolist with strbuf_getline()
In read_rebase_todolist() the files $GIT_DIR/rebase-merge/done and
$GIT_DIR/rebase-merge/git-rebase-todo are read to collect status
information.
The access to this file should always happen via git rebase, e.g. via
"git rebase -i" or "git rebase --edit-todo". We can assume, that this
interface handles the preprocessing of whitespaces, especially CRLFs
correctly. Thus in this codepath we can remove the call to strbuf_trim().
For documenting the input as expecting "text" input, strbuf_getline_lf()
is still replaced by strbuf_getline().
Signed-off-by: Moritz Neeb
---
wt-status.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index ab4f80d..8047cf2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname,
struct string_list *lines)
if (!f)
die_errno("Could not open file %s for reading",
git_path("%s", fname));
- while (!strbuf_getline_lf(&line, f)) {
+ while (!strbuf_getline(&line, f)) {
if (line.len && line.buf[0] == comment_line_char)
continue;
- strbuf_trim(&line);
if (!line.len)
continue;
abbrev_sha1_in_line(&line);
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/7] notes copy --stdin: split lines with string_list_split()
strbuf_split() has the unfortunate behavior of leaving the
separator character on the end of the split components, thus
placing the burden of manually removing the separator on the
caller. It's also heavyweight in that each split component is a
full-on strbuf. We need neither feature of strbuf_split() so
let's use string_list_split() instead since it removes the
separator character and returns an array of simple NUL-terminated
strings.
Helped-by: Eric Sunshine
Signed-off-by: Moritz Neeb
---
builtin/notes.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 706ec11..715fade 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -292,17 +292,16 @@ static int notes_copy_from_stdin(int force, const char
*rewrite_cmd)
while (strbuf_getline(&buf, stdin) != EOF) {
unsigned char from_obj[20], to_obj[20];
- struct strbuf **split;
+ struct string_list split = STRING_LIST_INIT_DUP;
int err;
- split = strbuf_split(&buf, ' ');
- if (!split[0] || !split[1])
+ string_list_split(&split, buf.buf, ' ', -1);
+ if (split.nr != 2)
die(_("Malformed input line: '%s'."), buf.buf);
- strbuf_rtrim(split[0]);
- if (get_sha1(split[0]->buf, from_obj))
- die(_("Failed to resolve '%s' as a valid ref."),
split[0]->buf);
- if (get_sha1(split[1]->buf, to_obj))
- die(_("Failed to resolve '%s' as a valid ref."),
split[1]->buf);
+ if (get_sha1(split.items[0].string, from_obj))
+ die(_("Failed to resolve '%s' as a valid ref."),
split.items[0].string);
+ if (get_sha1(split.items[1].string, to_obj))
+ die(_("Failed to resolve '%s' as a valid ref."),
split.items[1].string);
if (rewrite_cmd)
err = copy_note_for_rewrite(c, from_obj, to_obj);
@@ -312,11 +311,11 @@ static int notes_copy_from_stdin(int force, const char
*rewrite_cmd)
if (err) {
error(_("Failed to copy notes from '%s' to '%s'"),
- split[0]->buf, split[1]->buf);
+ split.items[0].string, split.items[1].string);
ret = 1;
}
- strbuf_list_free(split);
+ string_list_clear(&split, 0);
}
if (!rewrite_cmd) {
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()
The format of a line that is expected when copying notes via stdin
is "sha1 sha1". As this is text-only, strbuf_getline() should be used
instead of strbuf_getline_lf(), as documentation of this fact.
When reading with strbuf_getline() the trimming of split[1] can be
removed. It was necessary before to remove potential CRs inserted
through a dos editor.
Signed-off-by: Moritz Neeb
---
builtin/notes.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..706ec11 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char
*rewrite_cmd)
t = &default_notes_tree;
}
- while (strbuf_getline_lf(&buf, stdin) != EOF) {
+ while (strbuf_getline(&buf, stdin) != EOF) {
unsigned char from_obj[20], to_obj[20];
struct strbuf **split;
int err;
@@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char
*rewrite_cmd)
if (!split[0] || !split[1])
die(_("Malformed input line: '%s'."), buf.buf);
strbuf_rtrim(split[0]);
- strbuf_rtrim(split[1]);
if (get_sha1(split[0]->buf, from_obj))
die(_("Failed to resolve '%s' as a valid ref."),
split[0]->buf);
if (get_sha1(split[1]->buf, to_obj))
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()
The line read from the branch file is directly trimmed after reading with
strbuf_trim(). There is thus no logic expecting CR, so strbuf_getline_lf()
can be replaced by its CRLF counterpart.
Signed-off-by: Moritz Neeb
---
remote.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index fc02698..77e011a 100644
--- a/remote.c
+++ b/remote.c
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)
if (!f)
return;
- strbuf_getline_lf(&buf, f);
+ strbuf_getline(&buf, f);
fclose(f);
strbuf_trim(&buf);
if (!buf.len) {
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/7] bisect: read bisect paths with strbuf_getline()
The file BISECT_NAMES is written by "git rev-parse --sq-quote" via
sq_quote_argv() when starting a bisection. It can contain pathspecs
to narrow down the search. When reading it back, it should be expected that
sq_dequote_to_argv_array() is able to parse this file. In fact, the
previous commit ensures this.
As the content is of type "text", that means there is no logic expecting
CR, strbuf_getline_lf() will be replaced by strbuf_getline().
Apart from whitespace added and removed in quote.c, no other whitespaces
are expected. While it is technically possible, we have never advertised
this file to be editable by user, or encouraged them to do so. As a
consequence, the parsing of BISECT_NAMES is tightened by removing
strbuf_trim().
For the case that this file is modified nonetheless, in an invalid way
such that dequoting fails, the error message is broadened to both cases:
bad quoting and unexpected whitespace.
Helped-by: Junio C Hamano
Signed-off-by: Moritz Neeb
---
bisect.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/bisect.c b/bisect.c
index 7996c29..f63aa10 100644
--- a/bisect.c
+++ b/bisect.c
@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)
if (!fp)
die_errno("Could not open file '%s'", filename);
- while (strbuf_getline_lf(&str, fp) != EOF) {
- strbuf_trim(&str);
+ while (strbuf_getline(&str, fp) != EOF) {
if (sq_dequote_to_argv_array(str.buf, array))
- die("Badly quoted content in file '%s': %s",
+ die("Badly quoted content or unexpected whitespace in
file '%s': %s",
filename, str.buf);
}
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.8.0-rc0
On 2/27/2016 0:41, Junio C Hamano wrote: * Some calls to strcpy(3) triggers a false warning from static analysers that are less intelligent than humans, and reducing the number of these false hits helps us notice real issues. A few calls to strcpy(3) in test-path-utils that are already safe has been rewritten to avoid false wanings. * Some calls to strcpy(3) triggers a false warning from static analysers that are less intelligent than humans, and reducing the number of these false hits helps us notice real issues. A few calls to strcpy(3) in "git rerere" that are already safe has been rewritten to avoid false wanings. This is a duplicate. Regards, Sebastian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] compat/mingw: brown paper bag fix for 50a6c8e
On Mon, Feb 29, 2016 at 07:30:02AM +0100, Torsten Bögershausen wrote: > I haven't digged further, > but trying to verify t9115 under Windows gave this: > > compat/mingw.c: In function 'mingw_spawnve_fd': > compat/mingw.c:1072:10: warning: implicit declaration of function > 'xmalloc_array' [-Wimplicit-function-declaration] > wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t)); > ^ Yikes. -- >8 -- Subject: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e Commit 50a6c8e (use st_add and st_mult for allocation size computation, 2016-02-22) fixed up many xmalloc call-sites including ones in compat/mingw.c. But I screwed up one of them, which was half-converted to ALLOC_ARRAY, using a very early prototype of the function. And I never caught it because I don't build on Windows. Signed-off-by: Jeff King --- I think this means "master" is broken for mingw builds. Sorry, Windows people, for breaking your build. I'm happy to hold back such repo-wide cleanups from the mingw code in the future, since I can't actually compile them. But the flipside is that if I _do_ improve things, you don't get the benefit until somebody manually ports it over. compat/mingw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index cfedcf9..54c82ec 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen free(quoted); } - wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t)); + ALLOC_ARRAY(wargs, st_add(st_mult(2, args.len), 1)); xutftowcs(wargs, args.buf, 2 * args.len + 1); strbuf_release(&args); -- 2.8.0.rc0.276.gddf4100 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Trouble Cloning Git remote repository
On Mon, Feb 29, 2016 at 08:20:46AM +0700, Duy Nguyen wrote:
> On Mon, Feb 29, 2016 at 12:48 AM, Fred's Personal
> wrote:
> > Duy,
> >
> > Thanks for advice, here is the result of executing the lines you suggested:
> >
> > user1@Host1 MINGW64 ~/gitrepository (master)
> > $ export GIT_TRACE=1
> >
> > user1@Host1 MINGW64 ~/gitrepository (master)
> > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > 12:33:47.928365 git.c:348 trace: built-in: git 'clone' '-v'
> > 'ssh://user1@Host2/srv/centralrepo'
> > Cloning into 'centralrepo'...
> > 12:33:48.022110 run-command.c:343 trace: run_command: 'C:\Program
> > Files (x86)\PuTTY\plink.exe' 'user1@Host2' 'git-upload-pack
> > '\''/srv/centralrepo'\'''
>
> This command looks good to me. So I have no clue what goes wrong :-)
> Maybe you can execute this command directly, with more plink debugging
> options or something. You don't have to run git-upload-pack. Just
> spawn a shell. Another option is try something other than plink (does
> git-for-windows come with ssh.exe?)
On Windows it's probably going through mingw_spawnve_fd() which
reassembles a quoted command line from the individual arguments. It
would be interesting to see what gets passed to CreateProcessW().
> > ##>>>Lines from $HOME/.bashrc (See below, removed here for clarity)
> >
> > + user1@Host2 git-upload-pack /srv/centralrepo
> > bash: user1@Host2: command not found
> > fatal: Could not read from remote repository.
> >
> > Please make sure you have the correct access rights
> > and the repository exists.
> >
> >
> > Regards,
> > Fred
> >
> > freddie...@optonline.net
> >
> >
> > -Original Message-
> > From: Duy Nguyen [mailto:pclo...@gmail.com]
> > Sent: Saturday, February 27, 2016 4:36 AM
> > To: Fred's Personal
> > Cc: Git Mailing List
> > Subject: Re: Trouble Cloning Git remote repository
> >
> > On Sat, Feb 27, 2016 at 6:03 AM, Fred's Personal
> > wrote:
> >> $ git clone -v ssh://user1@Host2/srv/centralrepo Cloning into
> >> 'centralrepo'...
> >Lines from $HOME/.bashrc
> >> + export
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr
> >> /games
> >> :/usr/local/games
> >> +
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr
> >> /games
> >> :/usr/local/games
> >> + PROMPT_COMMAND=
> >> + CDPATH=
> >> + '[' '' = yes ']'
> >> + PS1='${debian_chroot:+($debian_chroot)}\u:\W\$ '
> >> + export GIT_TRACE_PACKET=1
> >> + GIT_TRACE_PACKET=1
> >> + export GIT_TRACE=1
> >> + GIT_TRACE=1
> >End of Lines from $HOME/.bashrc
> >> ## WHERE DOES The following line COME FROMWhat Script spits out
> >> this line
> >> + user1@Host2 git-upload-pack /srv/centralrepo
> >
> > Try set GIT_TRACE=1 at the clone line, I have a feeling that this line
> > should be "ssh user@Host2..." but "ssh" is missing.
> >
> > $ export GIT_TRACE=1
> > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > --
> > Duy
> >
> >
> > -
> > No virus found in this message.
> > Checked by AVG - www.avg.com
> > Version: 2016.0.7442 / Virus Database: 4537/11702 - Release Date: 02/26/16
> >
> >
>
>
>
> --
> Duy
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fetch-pack: fix object_id of exact sha1
On Sun, Feb 28, 2016 at 11:29:39AM -0800, Junio C Hamano wrote:
> Gabriel Souza Franco writes:
>
> >>> struct object_id oid;
> >>>
> >>> - if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
> >>> - name += GIT_SHA1_HEXSZ + 1;
> >>> - else
> >>> + if (get_oid_hex(name, &oid))
> >>> oidclr(&oid);
> >>> + else if (name[GIT_SHA1_HEXSZ] == ' ')
> >>> + name += GIT_SHA1_HEXSZ + 1;
> >>
> >> This makes sense to me. I wonder if we should be more particular about
> >> the pure-sha1 case consuming the whole string, though. E.g., if we get:
> >>
> >> 1234567890123456789012345678901234567890-bananas
> >>
> >> that should probably not have sha1 1234...
> >>
> >> I don't think it should ever really happen in practice, but it might be
> >> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither
> >> space nor '\0'.
> >
> > Right. What kind of complaining? Is doing oidclr(&oid) and letting it
> > fail elsewhere enough?
>
> I think that is the most sensible. After all, the first get-oid-hex
> expects to find a valid 40-hex object name at the beginning of line,
> and oidclr() is the way for it to signal a broken input, which is
> how the callers of this codepath expects errors to be caught.
Actually, I think we _don't_ want to signal an error here, but checking
for '\0' is still the right thing to do.
Once upon a time, fetch-pack took just the names of refs, like:
git fetch-pack $remote refs/heads/foo
and the same format was used for --stdin. Then in 58f2ed0 (remote-curl:
pass ref SHA-1 to fetch-pack as well, 2013-12-05), it learned to take
"$sha1 $ref". But if we didn't see a sha1, then we continued to treat it
as a refname.
This patch adds a new format, just "$sha1". So if get_oid_hex() succeeds
_and_ we see '\0', we know we have that case. But if we don't see '\0',
then we should assume it's a refname (e.g., "1234abcd...-bananas").
I think in practice it shouldn't matter much, as callers should be
feeding fully qualified refs (and we document this). However, we still
want to distinguish so that we give the correct error ("no such remote
ref 1234abcd...-bananas", not "whoops, the other side doesn't have
1234abcd").
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e
Thanks for the fast-patch.
I manually copied the patch, But there are more probles, it seems.
$ git diff
diff --git a/compat/mingw.c b/compat/mingw.c
index cfedcf9..b1163f2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char
**argv, char **deltaen
free(quoted);
}
- wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
+ wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
xutftowcs(wargs, args.buf, 2 * args.len + 1);
strbuf_release(&args);
tb@torbogwm MINGW64 ~/Users/tb/projects/git/tb ((2273582...))
$ make
CC compat/mingw.o
In file included from compat/mingw.c:1:0:
compat/mingw.c: In function 'mingw_spawnve_fd':
compat/../git-compat-util.h:769:60: error: invalid type argument of unary '*'
(have 'size_t {aka long long unsigned int}')
#define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
^
compat/mingw.c:1072:10: note: in expansion of macro 'ALLOC_ARRAY'
wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), sizeof(wchar_t));
^
Makefile:1948: recipe for target 'compat/mingw.o' failed
make: *** [compat/mingw.o] Error 1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] fetch-pack: fix object_id of exact sha1
On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:
> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
>
> Signed-off-by: Gabriel Souza Franco
> ---
>
> Not the cleanest conditional I've ever written, but it should handle
> all cases correctly.
I think it does. But I wonder if it wouldn't be more readable to cover
the three formats independently, like:
if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {
/* , find refname */
name += GIT_SHA1_HEXSZ + 1;
} else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\0')
{
/* , leave sha1 as name */
} else {
/* , clear any cruft from get_oid_hex */
oidclr(&ref->old_oid);
}
And as a bonus you get rid of the separate "oid". That does call into
get_oid_hex twice, but I doubt the performance impact is measurable.
We could also do:
if (!get_oid_hex(name, &ref->old_oid)) {
if (name[GIT_SHA1_HEXSZ] == ' ') {
/* , find refname */
name += GIT_SHA1_HEXSZ + 1;
} else if (name[GIT_SHA1_HEXSZ] == '\0') {
/* , leave sha1 as name */
} else {
/* , clear cruft from oid */
oidclr(&ref->old_oid);
}
} else {
/* , clear cruft from get_oid_hex */
oidclr(&ref->old_oid);
}
if you want to minimize the calls at the expense of having to repeat the
oidclr().
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e
On Mon, Feb 29, 2016 at 10:56:22AM +0100, Torsten Bögershausen wrote: > Thanks for the fast-patch. > > I manually copied the patch, But there are more probles, it seems. > > $ git diff > diff --git a/compat/mingw.c b/compat/mingw.c > index cfedcf9..b1163f2 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const > char **argv, char **deltaen > free(quoted); > } > > - wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), > sizeof(wchar_t)); > + wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), sizeof(wchar_t)); > xutftowcs(wargs, args.buf, 2 * args.len + 1); > strbuf_release(&args); Argh. Let me write "git commit -a" on the inside of my brown paper bag, so that I actually send out the fix sitting in my working tree, not the half-finished thing I ran "git add" on. -- >8 -- Subject: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e Commit 50a6c8e (use st_add and st_mult for allocation size computation, 2016-02-22) fixed up many xmalloc call-sites including ones in compat/mingw.c. But I screwed up one of them, which was half-converted to ALLOC_ARRAY, using a very early prototype of the function. And I never caught it because I don't build on Windows. Signed-off-by: Jeff King --- compat/mingw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index cfedcf9..54c82ec 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen free(quoted); } - wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t)); + ALLOC_ARRAY(wargs, st_add(st_mult(2, args.len), 1)); xutftowcs(wargs, args.buf, 2 * args.len + 1); strbuf_release(&args); -- 2.8.0.rc0.276.gddf4100 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git cat-file -e behavior
Hi the documentation for "git cat-file -e" says: > Suppress all output; instead exit with zero status if exists and is a valid object. However running it on non existing object will complain "fatal: Not a valid object name". $ git cat-file -e master:README.rst $ echo $? 0 $ git cat-file -e master:foo fatal: Not a valid object name master:foo $ echo $? 128 Is the output in this case expected? I'm currently running 2.7.0. -- Michal Čihař | http://cihar.com | http://blog.cihar.com signature.asc Description: OpenPGP digital signature
Re: What's cooking in git.git (Feb 2016, #07; Thu, 25)
On Sat, Feb 27, 2016 at 08:12:22AM +0100, Torsten Bögershausen wrote: > > Torsten, what is the compiler version (I don't have Apple compilers, but > > it seems plausible that older clang might have the same problem). > > That's machine is running Mac OS X 10.6, which is no longer supported > with updates. > > gcc --version > i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3) > Copyright (C) 2007 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Thanks. Out of curiosity, I tried to reproduce by with a build of gcc 4.2.1, to see if I could bisect. But it seems the toolchain is quite complex. After much munging, I managed to build a broken compiler (which I think is due to a much too-new version of bison, but I stopped digging). Your suggestion elsewhere in the thread to just use clang instead sounds good to me. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Compiler warning under cygwin/mingw (was: fix for 50a6c8e)
That compiles OK, thanks. Sorry for high-jacking this thread, but while compiling under CYGWIN, found one warning: LINK git-credential-store.exe CC daemon.o daemon.c: In function ‘drop_privileges’: daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ [-Wimplicit-function-declaration] if (cred && (initgroups(cred->pass->pw_name, cred->gid) || ^ t9115 doesn't pass, NTFS doesn't like the non-UTF filenames, it seams. Probably the same problem as under Mac OS /HFS+- --- And MINGW is not happy for other reasons: builtin/rev-parse.c: In function 'cmd_rev_parse': builtin/rev-parse.c:775:12: warning: implicit declaration of function 'realpath' [-Wimplicit-function-declaration] if (!realpath(gitdir, absolute_path)) ^ CC builtin/revert.o CC builtin/write-tree.o LINK git.exe builtin/rev-parse.o: In function `cmd_rev_parse': C:\Users\tb\projects\git\tb/builtin/rev-parse.c:775: undefined reference to `realpath' collect2.exe: error: ld returned 1 exit status Makefile:1725: recipe for target 'git.exe' failed make: *** [git.exe] Error 1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Compiler warning under cygwin/mingw (was: fix for 50a6c8e)
On Mon, Feb 29, 2016 at 11:40:59AM +0100, Torsten Bögershausen wrote: > That compiles OK, thanks. > > Sorry for high-jacking this thread, but while compiling under CYGWIN, > found one warning: > >LINK git-credential-store.exe > CC daemon.o > daemon.c: In function ‘drop_privileges’: > daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ > [-Wimplicit-function-declaration] > if (cred && (initgroups(cred->pass->pw_name, cred->gid) || Interesting that it doesn't later complain in the link step. :) You should probably be compiling with the NO_INITGROUPS knob on that platform. > t9115 doesn't pass, NTFS doesn't like the non-UTF filenames, it seams. > Probably the same problem as under Mac OS /HFS+- > --- No comment from me on that one. > And MINGW is not happy for other reasons: > > builtin/rev-parse.c: In function 'cmd_rev_parse': > builtin/rev-parse.c:775:12: warning: implicit declaration of function > 'realpath' [-Wimplicit-function-declaration] >if (!realpath(gitdir, absolute_path)) > ^ I guess you're building "pu"; that is only in sg/completion-updates. I don't know if our custom real_path() would suffice there. You might want to ping the author. The patch is: http://article.gmane.org/gmane.comp.version-control.git/287462 -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"
On Fri, Feb 26, 2016 at 06:34:49PM -0500, Jeff King wrote:
> > So any implementation would probably have to either:
> >
> > - make two passes over the options, first figuring out
> > whether we need a git-dir, and then actually handling
> > the options. That's possible, but it's probably not
> > worth the trouble.
> >
> > - call setup_git_directory() on the fly when an option
> > needs it; that requires annotating all of the options,
> > and there are a considerable number of them.
>
> Having just sent this, of course, it occurs to me that a loop like:
>
>setup_git_directory_gently(&nongit);
>for (i = 0; i < argc; i++) {
> if (!strcmp(argv[i], "--local-env-vars"))
> ... and other nongit-ok options ...
>
> if (nongit)
> die("need a repo");
>
> if (!strcmp(argv[i], "--git-dir"))
> ... and other options ...
>}
Actually, it is even easier than that. Those options don't care whether
they are in a repo or not, so we can just wait to call
setup_git_directory() in the loop.
IOW, I think my 2/2 should be replaced with this:
-- >8 --
Subject: [PATCH] rev-parse: let some options run outside repository
Once upon a time, you could use "--local-env-vars" and
"--resolve-git-dir" outside of any git repository, but they
had to come first on the command line. Commit 68889b4
(rev-parse: remove restrictions on some options, 2013-07-21)
put them into the normal option-parsing loop, fixing the
latter. But it inadvertently broke the former, as we call
setup_git_directory() before starting that loop.
We can note that those options don't care even conditionally
about whether we are in a git repo. So it's fine if we
simply wait to setup the repo until we see an option that
needs it.
However, there is one special exception we should make:
historically, rev-parse will set up the repository and read
config even if there are _no_ options. Some of the
tests in t1300 rely on this to check "git -c $config"
parsing. That's not mirroring real-world use, and we could
tweak the test. But t0002 uses a bare "git rev-parse" to
check "are we in a git repository?". It's plausible that
real-world scripts are relying on this.
So let's cover this case specially, and treat an option-less
"rev-parse" as "see if we're in a repo".
Signed-off-by: Jeff King
---
builtin/rev-parse.c | 50 +--
t/t1515-rev-parse-outside-repo.sh | 4 ++--
2 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index cf8487b..c961b74 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -505,6 +505,7 @@ N_("git rev-parse --parseopt [] -- [...]\n"
int cmd_rev_parse(int argc, const char **argv, const char *prefix)
{
int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
+ int did_repo_setup = 0;
int has_dashdash = 0;
int output_prefix = 0;
unsigned char sha1[20];
@@ -528,11 +529,40 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
}
}
- prefix = setup_git_directory();
- git_config(git_default_config, NULL);
+ /* No options; just report on whether we're in a git repo or not. */
+ if (argc == 1) {
+ setup_git_directory();
+ git_config(git_default_config, NULL);
+ return 0;
+ }
+
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
+ if (!strcmp(arg, "--local-env-vars")) {
+ int i;
+ for (i = 0; local_repo_env[i]; i++)
+ printf("%s\n", local_repo_env[i]);
+ continue;
+ }
+ if (!strcmp(arg, "--resolve-git-dir")) {
+ const char *gitdir = argv[++i];
+ if (!gitdir)
+ die("--resolve-git-dir requires an argument");
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die("not a gitdir '%s'", argv[i]);
+ puts(gitdir);
+ continue;
+ }
+
+ /* The rest of the options require a git repository. */
+ if (!did_repo_setup) {
+ prefix = setup_git_directory();
+ git_config(git_default_config, NULL);
+ did_repo_setup = 1;
+ }
+
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
@@ -706,12 +736,6 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
add_ref_exclusion(&ref_excludes, arg + 10);
continue;
}
- if (!strcmp(ar
Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"
On Sat, Feb 27, 2016 at 07:53:02PM -0500, Eric Sunshine wrote: > > then it must receive the two lines of output in the correct > > s/correct/& order/ I fixed this and all of the other typos by switching to a patch that needs about one tenth as much explanation. :) I'm sure it's not possible that I introduced any new ones... -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"
On Sat, Feb 27, 2016 at 12:25:11PM +, John Keeping wrote:
> On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:
> > This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
> [snip]
> > The original patch was not spurred by an actual bug report,
> > but by an observation[1] that was essentially "eh, this
> > looks unnecessarily restrictive". It _is_ restrictive, but
> > it turns out to be necessarily so. :)
>
> The aim of the original series was to improve the documentation, so I
> don't think it's unreasonable to consider this a regression and revert
> the functional change. Although I think we can improve the behaviour
> slightly (see below).
Thanks for looking this over. I agree that the changes you suggested
would be an improvement over what I posted. But I tried out the
alternate plan to handle the repo-setup more gracefully inside the loop.
I think that looks much simpler (at the very least, I had to spend a lot
fewer words trying to justify it in the commit message!).
And that makes most of your suggestions obsolete. I'll go through
them...
> > +--local-env-vars::
> > + List the GIT_* environment variables that are local to the
> > + repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
> > + Only the names of the variables are listed, not their value,
> > + even if they are set.
>
> I think we should add:
>
> No other arguments may be supplied.
So now you can give other arguments. I doubt it's a _good idea_ to do
so, but you could certainly do:
git rev-parse --git-dir --local-env-vars
if you wanted to. You can even do the opposite, and I guess it would be
correct as long as you popped the final line off the end as --git-dir.
So I guess we could restrict it, but I don't think it's an issue in
practice, and it does technically work.
> > +--resolve-git-dir ::
> > + Check if is a valid repository or a gitfile that
> > + points at a valid repository, and print the location of the
> > + repository. If is a gitfile then the resolved path
> > + to the real repository is printed.
>
> Again I think this should say that only the `path` argument may be
> supplied.
This one I think is more reasonable to use along with other options. Or
you could even pass a sequence of:
git rev-parse --resolve-git-dir foo --resolve-git-dir bar
Again, I doubt it's very useful in practice, but it does the obvious
thing (whereas with my original patch, it silently ignored subsequent
options).
> > + if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {
>
> Maybe:
>
> if (argc > 1 && !strcmp("--local-env-vars", argv[1])) {
> if (argc != 2)
> die("--local-env-vars must be the only argument");
>
> since the behaviour of:
>
> $ git rev-parse --local-env-vars --
> --local-env-vars
> --
>
> is quite surprising.
This now does what you'd expect (it's probably not _useful_, but at
least it isn't horrifying and surprising, like the v1 behavior).
> > + if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {
>
> This is less bad, but again it might be nice to provide a better error
> if the path argument isn't supplied.
This one is OK now, too.
> > @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const
> > char *prefix)
> > add_ref_exclusion(&ref_excludes, arg + 10);
> > continue;
> > }
> > - if (!strcmp(arg, "--local-env-vars")) {
>
> What about leaving this in and replacing the body of the if statement
> with:
>
> die("--local-env-vars must be the first argument");
>
> ? I expect this will significantly reduce debugging time if anyone is
> relying on the current behaviour.
The new version I sent covers this, too (and it does the right thing).
Thanks for a careful review of the corner cases. Even though my response
to all of them is "yeah, but your suggestion is now obsolete", it makes
me feel better about the v2 patch to see that it magically clears up all
of these issues.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
GIT_TRACE_PERFORMANCE and pager
Hi, It looks like setting GIT_TRACE_PERFORMANCE to 1 or 2 (for stdout or stderr) does not always work well with commands that use a pager, for example: - > GIT_TRACE_PERFORMANCE=2 git log -1 commit f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e Author: Junio C Hamano Date: Fri Feb 26 13:45:26 2016 -0800 Git 2.8-rc0 Signed-off-by: Junio C Hamano - In the above the GIT_TRACE_PERFORMANCE output is missing. When I use "--no-pager", I get the GIT_TRACE_PERFORMANCE output: - > GIT_TRACE_PERFORMANCE=2 git --no-pager log -1 commit f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e Author: Junio C Hamano Date: Fri Feb 26 13:45:26 2016 -0800 Git 2.8-rc0 Signed-off-by: Junio C Hamano 12:16:31.258462 trace.c:420 performance: 0.001415428 s: git command: 'git' '--no-pager' 'log' '-1' - Setting GIT_TRACE to 1 or 2 seems to work, but maybe it is because it outputs stuff at the beginning of the process and not at the end. Cheers, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GIT_TRACE_PERFORMANCE and pager
On Mon, Feb 29, 2016 at 12:25:49PM +0100, Christian Couder wrote: > Setting GIT_TRACE to 1 or 2 seems to work, but maybe it is because it > outputs stuff at the beginning of the process and not at the end. Yeah, I think so. Try this (on Linux): $ GIT_PAGER='sed s/^/paged:/' \ GIT_TRACE_PERFORMANCE=1 \ strace -f -e write -e 'signal=!sigchld' git -p rev-parse strace: Process 31155 attached strace: Process 31156 attached [pid 31156] +++ exited with 0 +++ [pid 31155] +++ exited with 0 +++ write(2, "06:32:30.486995 [pid=31154] trac"..., 114) = -1 EBADF (Bad file descriptor) write(2, "Could not trace into fd given by"..., 99) = -1 EBADF (Bad file descriptor) +++ exited with 0 +++ We redirect stderr to the pager (note that GIT_TRACE=1 still goes to stderr; it never goes to stdout). We wait() on the pager process before we exit the main git process, and we don't print the performance stats until atexit(). So by the time we get there, the pager must be dead, and the pipe descriptor is closed (I'm actually kind of surprised we don't get EPIPE, but it looks like we close the descriptors in wait_for_pager()). One workaround is something like: GIT_TRACE_PERFORMANCE=3 3>&2 git ... though I guess I'd question whether trace-performance is interesting at all for a paged command, since it is also counting the length of time you spend looking at the pager waiting to hit "q". -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cat-file -e behavior
On Mon, Feb 29, 2016 at 10:58:29AM +0100, Michal Čihař wrote:
> the documentation for "git cat-file -e" says:
>
> > Suppress all output; instead exit with zero status if exists
> and is a valid object.
>
> However running it on non existing object will complain "fatal: Not a
> valid object name".
>
> $ git cat-file -e master:README.rst
> $ echo $?
> 0
> $ git cat-file -e master:foo
> fatal: Not a valid object name master:foo
> $ echo $?
> 128
>
> Is the output in this case expected?
>
> I'm currently running 2.7.0.
It looks like it has been this way forever. The first thing we do with
the object is resolve its name to a sha1, and that's where the error you
see comes from. And then we actually check whether we have the object.
I think the intended use was to feed it a sha1 to see if it exists. Then
the name-resolution step is a noop.
I'm not sure if the behavior you are seeing is all that bad (the
documentation could be read as suppressing the normal stdout output, but
error messages remain), but it would be easy to change:
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 54db118..afde169 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -35,6 +35,9 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name,
if (unknown_type)
flags |= LOOKUP_UNKNOWN_OBJECT;
+ if (opt == 'e')
+ return !has_sha1_file(sha1);
+
if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
die("Not a valid object name %s", obj_name);
@@ -58,9 +61,6 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name,
printf("%lu\n", size);
return 0;
- case 'e':
- return !has_sha1_file(sha1);
-
case 'c':
if (!obj_context.path[0])
die("git cat-file --textconv %s: must be
",
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git config --get-urlmatch does not set exit code 1 when no match is found
On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote: > My current woes are with multi-valued configuration values. More > specifically credential.helper > > The documentation of git config says that when a value is not matched > it should return 1. > > To reproduce make sure that credential.helper is not set. > > git config --get-urlmatch credential.helper http://somedomain:1234/ > echo %ERRORLEVEL% > 0 This isn't really addressing your question, but I should warn you that internally, the credential code _doesn't_ use the urlmatch infrastructure. It predates the urlmatch code, and was never converted (so basically only http.* uses urlmatch). I think there are some corner cases where the two behave differently. I'm not sure what you're using this for, but you may get surprising results. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cat-file -e behavior
Hi Dne 29.2.2016 v 12:44 Jeff King napsal(a): > It looks like it has been this way forever. The first thing we do with > the object is resolve its name to a sha1, and that's where the error you > see comes from. And then we actually check whether we have the object. > > I think the intended use was to feed it a sha1 to see if it exists. Then > the name-resolution step is a noop. I found this as best way to check whether file exists in branch. Checking git ls-tree output seems less error prone than checking return value of git cat-file -e... > I'm not sure if the behavior you are seeing is all that bad (the > documentation could be read as suppressing the normal stdout output, but > error messages remain), I understand this, that's why I'm asking whether it's expected output or not :-). -- Michal Čihař | http://cihar.com | http://blog.cihar.com signature.asc Description: OpenPGP digital signature
Re: Compiler warning under cygwin/mingw
On 29/02/16 10:40, Torsten Bögershausen wrote: > That compiles OK, thanks. > > > Sorry for high-jacking this thread, but while compiling under CYGWIN, > found one warning: > >LINK git-credential-store.exe > CC daemon.o > daemon.c: In function ‘drop_privileges’: > daemon.c:1136:15: warning: implicit declaration of function ‘initgroups’ > [-Wimplicit-function-declaration] > if (cred && (initgroups(cred->pass->pw_name, cred->gid) || Yeah, this has been there for a while - except it depends on which version of the header files you have. (Some may not see the warning). I have 'fixed' this twice before, then updated my installation and a change to the system headers broke it again! (The headers are currently 'broken'). So, I got tired of fixing it up and have left it a while - you never know a new update may fix it! ;-) [I personally don't use the git daemon on cygwin, so I don't know if this a problem in practice.] ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cat-file -e behavior
On Mon, Feb 29, 2016 at 01:16:34PM +0100, Michal Čihař wrote: > Dne 29.2.2016 v 12:44 Jeff King napsal(a): > > It looks like it has been this way forever. The first thing we do with > > the object is resolve its name to a sha1, and that's where the error you > > see comes from. And then we actually check whether we have the object. > > > > I think the intended use was to feed it a sha1 to see if it exists. Then > > the name-resolution step is a noop. > > I found this as best way to check whether file exists in branch. > Checking git ls-tree output seems less error prone than checking return > value of git cat-file -e... I think the usual way to do that would be: git rev-parse --verify $branch:$path >/dev/null which just resolves the name (not that what you are doing is wrong, I just think rev-parse is the way we usually do it in our scripts). That _will_ write a message to stderr when the name doesn't resolve. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git cat-file -e behavior
On Mon, Feb 29, 2016 at 06:44:55AM -0500, Jeff King wrote:
> [...]but it would be easy to change:
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 54db118..afde169 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -35,6 +35,9 @@ static int cat_one_file(int opt, const char *exp_type,
> const char *obj_name,
> if (unknown_type)
> flags |= LOOKUP_UNKNOWN_OBJECT;
>
> + if (opt == 'e')
> + return !has_sha1_file(sha1);
> +
> if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
> die("Not a valid object name %s", obj_name);
>
> @@ -58,9 +61,6 @@ static int cat_one_file(int opt, const char *exp_type,
> const char *obj_name,
> printf("%lu\n", size);
> return 0;
>
> - case 'e':
> - return !has_sha1_file(sha1);
> -
> case 'c':
> if (!obj_context.path[0])
> die("git cat-file --textconv %s: must be
> ",
This is wrong, of course. We still need to call get_sha1, but just need
to not die. So it's more like this (totally untested, naturally):
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 54db118..44add8c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -35,8 +35,11 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name,
if (unknown_type)
flags |= LOOKUP_UNKNOWN_OBJECT;
- if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
+ if (get_sha1_with_context(obj_name, 0, sha1, &obj_context)) {
+ if (opt == 'e')
+ return 1;
die("Not a valid object name %s", obj_name);
+ }
buf = NULL;
switch (opt) {
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e
Hi Peff, On Mon, 29 Feb 2016, Jeff King wrote: > I think this means "master" is broken for mingw builds. > > Sorry, Windows people, for breaking your build. I'm happy to hold back > such repo-wide cleanups from the mingw code in the future, since I can't > actually compile them. But the flipside is that if I _do_ improve > things, you don't get the benefit until somebody manually ports it over. No, I do not think that you need to hold back cleanups. We will catch such issues before long, anyway. Thanks for all your hard work! Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git config --get-urlmatch does not set exit code 1 when no match is found
@Peff Thank you for the heads up. I'm trying to find out if there are any credential helpers configured in the system that will be running tests. On the dedicated test machines that is not a problem but the developer machines are. Should I already post a pre-emptive email asking about the corner cases? More importantly for me is if there is a case where get-url would not show a match where git clone would. If git clone skips a configuration that config url-match doesn't then it's not so bad. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git 2.7.2.windows.1 fails to authenticate access to private repository over HTTPS
Hi Zac, On Mon, 29 Feb 2016, zacr wrote: > I have recently downloaded 64-bit Git for Windows (the file I downloaded > was "Git-2.7.2-64-bit.exe"). This reports a version string of "git > version 2.7.2.windows.1" > > Using it from the command line, after cloning a repository, I attempted > to "git pull" a private repository from GitHub. Are you using Git CMD? It is rather crucial to get such details right. > 15:55:58.615147 git.c:348 trace: built-in: git 'pull' > 15:55:58.617147 run-command.c:343 trace: run_command: 'fetch' > '--update-head-ok' > 15:55:58.633149 git.c:348 trace: built-in: git 'fetch' > '--update-head-ok' > 15:55:58.636149 run-command.c:343 trace: run_command: > 'git-remote-https' 'origin' 'https://github.com/username/repository.git' > 15:55:59.630248 run-command.c:343 trace: run_command: 'bash' '-c' 'cat > >/dev/tty && read -r line error: failed to execute prompt script (exit code 1) > fatal: could not read Username for 'https://github.com': Invalid argument > > > It looks like it's trying to run a new bash instance for me to enter my > password, which fails on Windows because I don't have bash installed. You do have bash installed. Unless you went out of your way to delete it after installing Git for Windows. Assuming that you used Git CMD to perform your test, the sad news is that I cannot reproduce your problem here. Let's first of all try to make a Minimal, Complete & Verifiable Example (MCVE). In other words, let's find the minimal set of steps with no prerequisites (such as local clones of private repositories) that *still* reproduce the bug. This is in general a Very Good Idea if you are reporting bugs because it not only shows that you are dedicated to see this bug fixed but also that you are not expecting only others to invest a ton of time into the bug fix. In your case, the problem is easily triggered without a local clone at all. A simple `git ls-remote https://github.com/user/repository.git` is enough, and would count as *Minimal* as per that MCVE acronym. Alas, when I issue this (with GIT_TRACE=1), I get this: -- snip -- C:\Users\me>set GIT_TRACE=1 C:\Users\me>git ls-remote https://github.com/user/repository.git 14:14:01.717354 git.c:348 trace: built-in: git 'ls-remote' 'https://github.com/user/repository.git' 14:14:01.718331 run-command.c:343 trace: run_command: 'git-remote-https' 'https://github.com/user/repository.git' 'https://github.com/user/repository.git' 14:14:02.599651 run-command.c:343 trace: run_command: 'bash' '-c' 'cat >/dev/tty && read -r line https://github.com': 123 14:14:06.822993 run-command.c:343 trace: run_command: 'bash' '-c' 'cat >/dev/tty && read -r -s line /dev/tty' Password for 'https://1...@github.com': remote: Invalid username or password. fatal: Authentication failed for 'https://github.com/user/repository.git/' -- snap -- So at first I thought maybe you called C:\Program Files\Git\mingw64\bin\git.exe directly, which is a user mistake that is unfortunately quite frequent, the appropriate entry point is instead C:\Program Files\Git\cmd\git.exe But even then, it succeeds for me (without having sh.exe nor bash.exe in the PATH): -- snip -- C:\Users\me>"\Program Files"\git\mingw64\bin\git ls-remote https://github.com/user/repository.git 14:22:44.320489 git.c:348 trace: built-in: git 'ls-remote' 'https://github.com/user/repository.git' 14:22:44.321470 run-command.c:343 trace: run_command: 'git-remote-https' 'https://github.com/user/repository.git' 'https://github.com/user/repository.git' 14:22:44.990258 run-command.c:343 trace: run_command: 'bash' '-c' 'cat >/dev/tty && read -r line https://github.com': 123 14:22:45.840206 run-command.c:343 trace: run_command: 'bash' '-c' 'cat >/dev/tty && read -r -s line /dev/tty' Password for 'https://1...@github.com': remote: Invalid username or password. fatal: Authentication failed for 'https://github.com/user/repository.git/' -- snap -- Maybe your setup has something funky going on that my setup lacks. Lastly, for the record, the best way to report bugs in Git for Windows is through the bug tracker at https://github.com/git-for-windows/git/issues. I tried to make this obvious on Git for Windows' home page at https://git-for-windows.github.io/#contribute. Maybe you have an idea how I could make it more obvious to users who prefer visiting StackOverflow over visiting Git for Windows' home page? Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GIT_TRACE_PERFORMANCE and pager
On Mon, Feb 29, 2016 at 12:39 PM, Jeff King wrote: > On Mon, Feb 29, 2016 at 12:25:49PM +0100, Christian Couder wrote: > >> Setting GIT_TRACE to 1 or 2 seems to work, but maybe it is because it >> outputs stuff at the beginning of the process and not at the end. > > Yeah, I think so. Try this (on Linux): > > $ GIT_PAGER='sed s/^/paged:/' \ > GIT_TRACE_PERFORMANCE=1 \ > strace -f -e write -e 'signal=!sigchld' git -p rev-parse > strace: Process 31155 attached > strace: Process 31156 attached > [pid 31156] +++ exited with 0 +++ > [pid 31155] +++ exited with 0 +++ > write(2, "06:32:30.486995 [pid=31154] trac"..., 114) = -1 EBADF (Bad file > descriptor) > write(2, "Could not trace into fd given by"..., 99) = -1 EBADF (Bad file > descriptor) > +++ exited with 0 +++ Yeah, I get the same thing. > We redirect stderr to the pager (note that GIT_TRACE=1 still goes to > stderr; it never goes to stdout). We wait() on the pager process before > we exit the main git process, and we don't print the performance stats > until atexit(). So by the time we get there, the pager must be dead, and > the pipe descriptor is closed (I'm actually kind of surprised we don't > get EPIPE, but it looks like we close the descriptors in > wait_for_pager()). > > One workaround is something like: > > GIT_TRACE_PERFORMANCE=3 3>&2 git ... Yeah, that works. > though I guess I'd question whether trace-performance is interesting at > all for a paged command, since it is also counting the length of time > you spend looking at the pager waiting to hit "q". In case of "GIT_TRACE_PERFORMANCE=2 git log -1", it doesn't count the time spent looking at the pager because the output is small, so 'less' exits immediately, and it could give the impression that GIT_TRACE_PERFORMANCE is not working. I am preparing a patch to Documentation/technical/api-trace.txt to warn about that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Change type of signed int flags to unsigned
“pattern” and “exclude” are two structs defined in attr.c and dir.h
respectively. Each contain a “flags” field of type int that takes on
values from the set of positive integers {1, 4, 8, 16}, that are
enumerated through the macro EXC_FLAG_*.
That the most significant bit (used to store the sign) of these two
fields is not used in any special way is observed from the fact that
the "flags" fields (accessed within attr.c, dir.c, and
builtin/check-ignore.c) are either checked for their values using the
& operator (e.g.: flags & EXC_FLAG_NODIR), or assigned a value of 0
first and then assigned any one of {1, 4, 8, 16} using the | operator
(e.g.: flags |= EXC_FLAG_NODIR). Hence, change the type of "flags"
to unsigned in both structs.
Furthermore, “flags” of both structs is passed by value or by reference
to the functions parse_exclude_pattern, match_basename and
match_pathname (declared in dir.h), that have an “int” or “int *” type
for "flags" in their signatures. To avoid implicit conversion to types,
or pointers to types, of different sign, change the type for “flags” to
“unsigned” and “unsigned *” in the respective function signatures.
And while we’re at it, document the "flags" field of "exclude" to
explicitly state the values it’s supposed to take on.
Signed-off-by: Saurav Sachidanand
---
This patch is for the suggested microproject for GSoC 2016 titled
"Use unsigned integral type for collection of bits." It is the third
patch for this project (technically second, considering only the
subject of the email) that incorporates changes to the commit message
suggested by Moritz Neeb and Eric Sunshine, and to some function
signatures suggested by Duy Nguyen. Thanks to them for their feedback.
attr.c | 2 +-
dir.c | 8
dir.h | 8
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index 086c08d..679e13c 100644
--- a/attr.c
+++ b/attr.c
@@ -124,7 +124,7 @@ struct pattern {
const char *pattern;
int patternlen;
int nowildcardlen;
- int flags; /* EXC_FLAG_* */
+ unsigned flags; /* EXC_FLAG_* */
};
/*
diff --git a/dir.c b/dir.c
index 552af23..82cec7d 100644
--- a/dir.c
+++ b/dir.c
@@ -459,7 +459,7 @@ int no_wildcard(const char *string)
void parse_exclude_pattern(const char **pattern,
int *patternlen,
- int *flags,
+ unsigned *flags,
int *nowildcardlen)
{
const char *p = *pattern;
@@ -500,7 +500,7 @@ void add_exclude(const char *string, const char *base,
{
struct exclude *x;
int patternlen;
- int flags;
+ unsigned flags;
int nowildcardlen;
parse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);
@@ -811,7 +811,7 @@ void add_excludes_from_file(struct dir_struct *dir, const
char *fname)
int match_basename(const char *basename, int basenamelen,
const char *pattern, int prefix, int patternlen,
- int flags)
+ unsigned flags)
{
if (prefix == patternlen) {
if (patternlen == basenamelen &&
@@ -836,7 +836,7 @@ int match_basename(const char *basename, int basenamelen,
int match_pathname(const char *pathname, int pathlen,
const char *base, int baselen,
const char *pattern, int prefix, int patternlen,
- int flags)
+ unsigned flags)
{
const char *name;
int namelen;
diff --git a/dir.h b/dir.h
index 3ec3fb0..e942b50 100644
--- a/dir.h
+++ b/dir.h
@@ -28,7 +28,7 @@ struct exclude {
int nowildcardlen;
const char *base;
int baselen;
- int flags;
+ unsigned flags; /* EXC_FLAG_* */
/*
* Counting starts from 1 for line numbers in ignore files,
@@ -229,10 +229,10 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir,
const char *pathname,
* attr.c:path_matches()
*/
extern int match_basename(const char *, int,
- const char *, int, int, int);
+ const char *, int, int, unsigned);
extern int match_pathname(const char *, int,
const char *, int,
- const char *, int, int, int);
+ const char *, int, int, unsigned);
extern struct exclude *last_exclude_matching(struct dir_struct *dir,
const char *name, int *dtype);
@@ -244,7 +244,7 @@ extern struct exclude_list *add_exclude_list(struct
dir_struct *dir,
extern int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen,
struct exclude_list *el, int
check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
-extern void parse_exclude_pattern(const char **string, int *patternlen, int
*flags, int *nowildcardlen);
Re: Compiler warning under cygwin/mingw (was: fix for 50a6c8e)
Hi,
> > And MINGW is not happy for other reasons:
> >
> > builtin/rev-parse.c: In function 'cmd_rev_parse':
> > builtin/rev-parse.c:775:12: warning: implicit declaration of function
> > 'realpath' [-Wimplicit-function-declaration]
> >if (!realpath(gitdir, absolute_path))
> > ^
>
> I guess you're building "pu"; that is only in sg/completion-updates. I
> don't know if our custom real_path() would suffice there. You might want
> to ping the author. The patch is:
>
> http://article.gmane.org/gmane.comp.version-control.git/287462
Oh, I was not aware that there is a custom real_path() that is
preferred over the system realpath(). I don't see why our real_path()
would not suffice, it even makes the code a tad shorter.
I will include the patch below in the reroll.
Best,
Gábor
>8
Subject: [PATCH] fixup! rev-parse: add '--absolute-git-dir' option
---
builtin/rev-parse.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 90a4dd6032c0..d6d9a82c77c4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -762,10 +762,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
if (!gitdir && !prefix)
gitdir = ".git";
if (gitdir) {
- char absolute_path[PATH_MAX];
- if (!realpath(gitdir,
absolute_path))
- die_errno(_("unable to
get absolute path"));
- puts(absolute_path);
+ puts(real_path(gitdir));
continue;
}
}
--
2.7.2.410.g92cb358
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Documentation: talk about pager in api-trace.txt
Signed-off-by: Christian Couder
---
Documentation/technical/api-trace.txt | 43 +++
1 file changed, 43 insertions(+)
diff --git a/Documentation/technical/api-trace.txt
b/Documentation/technical/api-trace.txt
index 097a651..a10b3a9 100644
--- a/Documentation/technical/api-trace.txt
+++ b/Documentation/technical/api-trace.txt
@@ -95,3 +95,46 @@ for (;;) {
}
trace_performance(t, "frotz");
+
+Bugs & Caveats
+--
+
+Some git commands, like `git log`, are run by default using a
+pager. In this case, stdout and stderr are redirected to the pager and
+are closed when the pager exits.
+
+If a GIT_TRACE* environment variable has been set to "1" or "2" to
+print traces on stderr, no trace output will be printed after the
+pager has exited.
+
+This can be annoying, because GIT_TRACE_PERFORMANCE by default prints
+the performance stats for the whole command at atexit() time which
+happens after the pager has exited.
+
+So the following command will print no performance stat:
+
+
+GIT_TRACE_PERFORMANCE=2 git log -1
+
+
+To overcome this problem, you can use one of the following
+work-arounds:
+
+ - redirect to another file descriptor which is redirected to stderr,
+like this:
+
+
+GIT_TRACE_PERFORMANCE=3 3>&2 git log -1
+
+
+ - redirect to a file specified by its absolute path, like this:
+
+
+GIT_TRACE_PERFORMANCE=/path/to/log/file git log -1
+
+
+ - use "--no-pager", like this:
+
+
+GIT_TRACE_PERFORMANCE=2 git --no-pager log -1
+
--
2.7.1.289.gf4cc727
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2016 Microproject
Hi, Should I make a patch for this and submit it for discussion on the mailing list? Regards, Sidhant Sharma [:tk] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2016 Microproject
Sidhant Sharma writes: > Hi, >> I didn't see anything going on for this one, but you may want to >> double-check with the ml's archives. >> > I checked the archives and there doesn't seem to be any active work on this. > I made the required changes and ran the test suite. Though all the > tests pass, there still are two queries I have. > First, I'm not quite sure what to put in the help message for the > options (--quiet, --stateless-rpc, --advertise-refs and > --reject-thin-pack-for-testing). They are currently undocumented. We sometimes have explicitly undocumented options (PARSE_OPT_HIDDEN) when they are used only internally to avoid polluting the end-user's UI. In this case, the command is anyway not meant for end-users so I think it would make sense to document them, but not necessarily within the the microproject. > Second, regarding the reject-thin-pack-for-testing option, currently > when the option is entered, `fix_thin` is unset > (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1736). > But using `OPT_BOOL(...)` for the same, the variable would instead be > set when the option is given. I think one solution can be to invert > `fix_thin` after calling `parse_options`. Am I going right so far? > Suggestions and corrections welcome. Or use OPT_SET_INT(..., 0) on a variable initialized to 1. > Should I make a patch for this and submit it for discussion on the mailing > list? On this list, it is indeed often more efficient to say "here's what I'm done. Any comments?" than "here's what I'm about to do". -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] http: honor no_http env variable to bypass proxy
From: Jiang Xin
Curl and its families honor several proxy related environment variables:
* http_proxy and https_proxy define proxy for http/https connections.
* no_proxy (a comma separated hosts) defines hosts bypass the proxy.
This command will bypass the bad-proxy and connect to the host directly:
no_proxy=* https_proxy=http://bad-proxy/ \
curl -sk https://google.com/
Before commit 372370f (http: use credential API to handle proxy auth...),
Environment variable "no_proxy" will take effect if the config variable
"http.proxy" is not set. So the following comamnd won't fail if not
behind a firewall.
no_proxy=* https_proxy=http://bad-proxy/ \
git ls-remote https://github.com/git/git
But commit 372370f not only read git config variable "http.proxy", but
also read "http_proxy" and "https_proxy" environment variables, and set
the curl option using:
curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
This caused "no_proxy" environment variable not working any more.
Set extra curl option "CURLOPT_NOPROXY" will fix this issue.
Signed-off-by: Jiang Xin
---
http.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/http.c b/http.c
index 1d5e3bb..69da445 100644
--- a/http.c
+++ b/http.c
@@ -70,6 +70,7 @@ static long curl_low_speed_limit = -1;
static long curl_low_speed_time = -1;
static int curl_ftp_no_epsv;
static const char *curl_http_proxy;
+static const char *curl_no_proxy;
static const char *http_proxy_authmethod;
static struct {
const char *name;
@@ -624,6 +625,11 @@ static CURL *get_curl_handle(void)
}
curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
+#if LIBCURL_VERSION_NUM >= 0x071304
+ var_override(&curl_no_proxy, getenv("NO_PROXY"));
+ var_override(&curl_no_proxy, getenv("no_proxy"));
+ curl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);
+#endif
}
init_curl_proxy_auth(result);
--
2.8.0.rc0.1.g9eb3984.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4.py: Make submit working on bare repository
On 28 February 2016 at 20:46, Amadeusz Żołnowski wrote: > > True. For now I have these cases covered by wrapper scripts. The minimum > I need from git-p4 is just not to fail on git submit from bare > repository which is covered by patch I have submitted. If I get my > solution enough testing, I'd think of transforming it into patch for > git-p4.py as well. Could you change the patch to add a command-line option to suppress the rebase? I think this would be a bit more obvious: instead of having some special magical behaviour kick-in on a bare repo, git-p4 just does what it's told on the command-line. It means that if we find another situation where we don't want to rebase, we don't have an ever-growing list of special-case circumstances, which could become hard to make sense of in future. Instead, the user (who hopefully knows better) just tells git-p4 what to do. Thanks! Luke -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2016 Microproject
>> First, I'm not quite sure what to put in the help message for the >> options (--quiet, --stateless-rpc, --advertise-refs and >> --reject-thin-pack-for-testing). > They are currently undocumented. We sometimes have explicitly > undocumented options (PARSE_OPT_HIDDEN) when they are used only > internally to avoid polluting the end-user's UI. > > In this case, the command is anyway not meant for end-users so I think > it would make sense to document them, but not necessarily within the the > microproject. So what may I put in the message parameter? I was thinking perhaps the option itself, without hyphens. Would that be correct? > >> Second, regarding the reject-thin-pack-for-testing option, currently >> when the option is entered, `fix_thin` is unset >> (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1736). >> But using `OPT_BOOL(...)` for the same, the variable would instead be >> set when the option is given. I think one solution can be to invert >> `fix_thin` after calling `parse_options`. Am I going right so far? >> Suggestions and corrections welcome. > Or use OPT_SET_INT(..., 0) on a variable initialized to 1. Okay, will do that. >> Should I make a patch for this and submit it for discussion on the mailing >> list? > On this list, it is indeed often more efficient to say "here's what I'm > done. Any comments?" than "here's what I'm about to do". > I'm really sorry, I'm not very familiar with mailing list etiquettes. I'll keep that in mind :) Thanks and regards, Sidhant Sharma [:tk] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2016 Microproject
Sidhant Sharma writes: >>> First, I'm not quite sure what to put in the help message for the >>> options (--quiet, --stateless-rpc, --advertise-refs and >>> --reject-thin-pack-for-testing). >> They are currently undocumented. We sometimes have explicitly >> undocumented options (PARSE_OPT_HIDDEN) when they are used only >> internally to avoid polluting the end-user's UI. >> >> In this case, the command is anyway not meant for end-users so I think >> it would make sense to document them, but not necessarily within the the >> microproject. > So what may I put in the message parameter? I was thinking > perhaps the option itself, without hyphens. Would that be > correct? If you use PARSE_OPT_HIDDEN, I think you don't need to specify a message. Otherwise, the documentation only has value if it contains more than just the option name, but that is the hard part if you're not familiar with the code. The best place to find documentation is in the history (git blame the file and see if the commit message introducing the option enlightens you). But that's why I said this didn't have to be part of the microproject: writting good doc requires a good understanding of the whole thing ... >>> Should I make a patch for this and submit it for discussion on the mailing >>> list? >> >> On this list, it is indeed often more efficient to say "here's what I'm >> done. Any comments?" than "here's what I'm about to do". >> > I'm really sorry, I'm not very familiar with mailing list etiquettes. > I'll keep that in mind :) No problem. It's OK to say what you do beforehand and to ask help. Just don't be surprised when you don't get much feedback on message not starting with [PATCH] ;-). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Feb 2016, #07; Thu, 25)
On Mon, Feb 29, 2016 at 5:18 AM, Jeff King wrote: > On Sat, Feb 27, 2016 at 08:12:22AM +0100, Torsten Bögershausen wrote: >> > Torsten, what is the compiler version (I don't have Apple compilers, but >> > it seems plausible that older clang might have the same problem). >> >> That's machine is running Mac OS X 10.6, which is no longer supported >> with updates. >> >> gcc --version >> i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3) >> Copyright (C) 2007 Free Software Foundation, Inc. >> This is free software; see the source for copying conditions. There is NO >> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > Thanks. Out of curiosity, I tried to reproduce by with a build of gcc > 4.2.1, to see if I could bisect. But it seems the toolchain is quite > complex. After much munging, I managed to build a broken compiler (which > I think is due to a much too-new version of bison, but I stopped > digging). > > Your suggestion elsewhere in the thread to just use clang instead sounds > good to me. :) If possible, for the moment, I'd prefer to hold off on that sledge-hammer approach of unconditionally making the build use clang. It would be nice to have a more detailed understanding of what exactly is triggering the Apple compiler bug, and I've been trying to find time to check it out on my old Mac. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GIT_TRACE_PERFORMANCE and pager
On Mon, Feb 29, 2016 at 3:39 AM, Jeff King wrote: > One workaround is something like: > > GIT_TRACE_PERFORMANCE=3 3>&2 git ... > > though I guess I'd question whether trace-performance is interesting at > all for a paged command, since it is also counting the length of time > you spend looking at the pager waiting to hit "q". Yup, exactly. I see Christian tried to add something to the documentation, but I think it is sufficient to suggest running commands with --no-pager. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git svn clones flawlessly on Windows, crashes silently on OS X - status of externals support?
Hello,
tl;dr: Can I get Git/SVN working on my MacBook if there are external references
in the repository? If not, are there alternatives that I can leverage?
I'm trying to use git svn to clone a repository that contains externals. On my
Windows7 machine it works perfectly. On my Mac (running OSX 10.11.3)
it fails silently after creating the empty local repository.
The posts that speak to some of the issues surrounding this are over two
years old, so I'm hoping things have changed.
I've tried with the versions of Git and SVN installed with XCode, and with
newer versions installed with HomeBrew and MacPorts. The latest version of
Git (2.7.2) is the one I've done my troubleshooting with.
I found the threads that speak of needing to link the SVN Perl modules into
directories being searched by the Perl runtime, and have done something
comparable with these:
sudo ln -s /opt/local/lib/perl5/vendor_perl/5.22/darwin-thread-multi-
2level/SVN /opt/local/lib/perl5/site_perl/5.22/darwin-thread-multi-2level/SVN
sudo ln -s /opt/local/lib/perl5/vendor_perl/5.22/darwin-thread-multi-
2level/auto /opt/local/lib/perl5/site_perl/5.22/auto
If I try with Git's debugger turned on, like so:
GIT_TRACE=1 git svn clone -s https://my-remote-repo
I get a bunch of redundant messages that look similar to the below, with
something suspicious happening right at the time Git encounters its first
external reference:
... many, many of these
Checked through r45000
16:49:19.677091 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.
branches-maxRev' '45000'
16:49:19.684111 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.tags-
maxRev' '45000'
Checked through r45100
16:49:19.721522 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.
branches-maxRev' '45100'
16:49:19.728460 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.tags-
maxRev' '45100'
16:49:19.784923 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.
reposRoot' 'https://a-remote-external'
... once this line appears, the "Checked through r*" messages disappear.
... The rest of the output looks like these. Then the process dies.
16:49:19.807647 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.
branches-maxRev' '45172'
16:49:19.814647 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.
tags-maxRev' '45172'
If I run with the Perl debugger like so:
PERLDB_OPTS="NonStop frame=5" /opt/local/bin/perl5.22 -d
$(git --exec-path)/git-svn clone -s https://my-remote-repository
I get the following stacktrace mush right before it crashes. It looks like
multiple processes are sending output to TTY so the stacktraces are
interleaved. There is a reference to Error::throw() down at the bottom that
I hope will be helpful.
in .=main::post_fetch_checkout() from /opt/local/libexec/git-core/
git-svn:387
in $=main::verify_ref('HEAD^0') from /opt/local/libexec/git-core/
git-svn:1716
in $=Git::command_oneline(ref(ARRAY), ref(HASH)) from /opt/
local/libexec/git-core/git-svn:1806
in @=Git::command_output_pipe(ref(ARRAY), ref(HASH)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:314
in @=Git::_command_common_pipe('-|', ref(ARRAY), ref(HASH)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:344
in @=Git::_maybe_self(ref(ARRAY), ref(HASH)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1561
in $=UNIVERSAL::isa(ref(ARRAY), 'Git') from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1549
in .=Git::_check_valid_cmd('rev-parse') from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1569 from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1598
in .=Git::_setup_git_cmd_env(undef) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1608
in .=Git::_execv_git_cmd('rev-parse', '--verify', 'HEAD^0') from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:1609
in @=Error::subs::with(ref(CODE)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:325
in $=Error::catch('Git::Error::Command', ref(CODE)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:325
in .=Error::subs::try(ref(CODE), ref(HASH)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:325
in .=CODE(0x7fa8a367bd40)() from
/opt/local/lib/perl5/vendor_perl/5.22/Error.pm:421
in .=Git::_cmd_close('rev-parse --verify HEAD^0', ref(GLOB)) from
/opt/local/lib/perl5/site_perl/5.22/Git.pm:319
in .=Error::throw('Git::Error::Command', 'rev-parse --verify HEAD^0'
, 128) from /opt/local/lib/perl5/site_perl/5.22/Git.pm:1640
in $=Git::Error::Command::new('Git::Error::Command', 'rev-parse
--verify HEAD^0', 128) from /opt/local/lib/perl5/vendor_perl/5.22/
Error.pm:184
in $=Error::new('Git::Error::Command', '-text', 'command returned
error', '-cmdline', 'rev-parse --verify HEAD^0', '-value', 128,
Re: What's cooking in git.git (Feb 2016, #07; Thu, 25)
Eric Sunshine writes: >> Your suggestion elsewhere in the thread to just use clang instead sounds >> good to me. :) > > If possible, for the moment, I'd prefer to hold off on that > sledge-hammer approach of unconditionally making the build use clang. > It would be nice to have a more detailed understanding of what exactly > is triggering the Apple compiler bug, and I've been trying to find > time to check it out on my old Mac. The (fixed) patch posted was "on OSX 10.6, use clang by default"; I think that for majority of the users, that is a reasonable thing to do. The result of the patch still allows those who want to see how badly old Gcc breaks to do so with "make CC=gcc", doesn't it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"
Jeff King writes: > IOW, I think my 2/2 should be replaced with this: This looks sensible. Don't we still want the documentation updates from the previous 2/2? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Invalid initial git gui message encoding
Hello.
Working with Git for Windows on git gui I noticed an issue on reading
initial message to git gui message prompt after squash or cherry-pick
conflict. When system encoding is not set to UTF-8 (and on my Windows
it is cp1250) squash/cherry pick conflict message gets invalid
encoding.
This issue and test case is more widely described on
https://github.com/git-for-windows/git/issues/664 . After I set
encoding to UTF-8 using fconfigure, the problem seems to be fixed.
I tried it also on Linux, but I cannot reproduce this problem as its
default encoding is utf-8. I verified it using simple tcl script:
"puts [encoding system]". On Windows I get:
$ tclsh enc.tcl
cp1250
on Linux:
$ tclsh enc.tcl
utf-8
It may be not a big issue for Linux, but I think it is worth to have
it merged in case of somebody had other encoding than UTF-8. This fix
has been already merged into Git for Windows.
As stated in Documentation/SubmittingPatches, this patch is based on
git://repo.or.cz/git-gui.git.
BTW. Is there any example of writing regression tests for git-gui?
Signed-off-by: yaras
---
git-gui.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/git-gui.sh b/git-gui.sh
index 11048c7..1ed5185 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1616,11 +1616,13 @@ proc run_prepare_commit_msg_hook {} {
if {[file isfile [gitdir MERGE_MSG]]} {
set pcm_source "merge"
set fd_mm [open [gitdir MERGE_MSG] r]
+ fconfigure $fd_mm -encoding utf-8
puts -nonewline $fd_pcm [read $fd_mm]
close $fd_mm
} elseif {[file isfile [gitdir SQUASH_MSG]]} {
set pcm_source "squash"
set fd_sm [open [gitdir SQUASH_MSG] r]
+ fconfigure $fd_sm -encoding utf-8
puts -nonewline $fd_pcm [read $fd_sm]
close $fd_sm
} else {
--
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count
Jacob Keller writes:
> From: Jacob Keller
>
> git submodule--helper clone usage specified that paths come after the --
> as a sequence. However, the actual implementation does not, and requires
> only a single path passed in via --path. In addition, argc was
> unchecked. (allowing arbitrary extra arguments that were silently
> ignored).
>
> Fix the usage description to match implementation. Add an argc check to
> enforce no extra arguments.
The above sounds very sensible.
> Fix a bug in the argument passing in
> git-submodule.sh which would pass --reference and --depth as empty
> strings when they were unused, resulting in extra argc after parsing
> options.
This does make sense but it is an unrelated fix. Perhaps split this
patch into two?
> Signed-off-by: Jacob Keller
> ---
> builtin/submodule--helper.c | 5 -
> git-submodule.sh| 4 ++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f4c3eff179b5..072d9bbd12a8 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -187,13 +187,16 @@ static int module_clone(int argc, const char **argv,
> const char *prefix)
> const char *const git_submodule_helper_usage[] = {
> N_("git submodule--helper clone [--prefix=] [--quiet] "
> "[--reference ] [--name ] [--url ]"
> -"[--depth ] [--] [...]"),
> +"[--depth ] [--path ]"),
> NULL
> };
>
> argc = parse_options(argc, argv, prefix, module_clone_options,
>git_submodule_helper_usage, 0);
>
> + if (argc)
> + usage(*git_submodule_helper_usage);
> +
That asterisk looks very unusual and wanting to be future-proofed
(i.e. who says that only the first entry matters?). Should't this
be calling usage_with_options()?
> strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> sm_gitdir = strbuf_detach(&sb, NULL);
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9bc5c5f94d1d..2dd29b3df0e6 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -347,7 +347,7 @@ Use -f if you really want to add it." >&2
> echo "$(eval_gettext "Reactivating local git
> directory for submodule '\$sm_name'.")"
> fi
> fi
> - git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix
> "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo"
> "$reference" "$depth" || exit
> + git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix
> "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo"
> ${reference:+"$reference"} ${depth:+"$depth"} || exit
> (
> clear_local_git_env
> cd "$sm_path" &&
> @@ -709,7 +709,7 @@ Maybe you want to use 'update --init'?")"
>
> if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
> then
> - git submodule--helper clone ${GIT_QUIET:+--quiet}
> --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference"
> "$depth" || exit
> + git submodule--helper clone ${GIT_QUIET:+--quiet}
> --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url"
> ${reference:+"$reference"} ${depth:+"$depth"} || exit
> cloned_modules="$cloned_modules;$name"
> subsha1=
> else
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()
On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb wrote:
> The format of a line that is expected when copying notes via stdin
> is "sha1 sha1". As this is text-only, strbuf_getline() should be used
> instead of strbuf_getline_lf(), as documentation of this fact.
>
> When reading with strbuf_getline() the trimming of split[1] can be
> removed. It was necessary before to remove potential CRs inserted
> through a dos editor.
s/dos/DOS/
This may not be worth a re-roll, but the suggestion of my v3 review
was to keep both rtrim's in this patch and then remove them in the
next patch when converting to string_list_split(). A benefit of doing
so is that you can then drop the above paragraph altogether, and both
patches become simpler (description and content), thus easier to
review.
If you do elect to keep things the way they are, then (as mentioned in
my v2 review) it would be helpful for the above paragraph to explain
that strbuf_split() leave the "terminator" on the split elements, thus
clarifying why the rtrim() of split[0] is still needed.
> Signed-off-by: Moritz Neeb
> ---
> builtin/notes.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index ed6f222..706ec11 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char
> *rewrite_cmd)
> t = &default_notes_tree;
> }
>
> - while (strbuf_getline_lf(&buf, stdin) != EOF) {
> + while (strbuf_getline(&buf, stdin) != EOF) {
> unsigned char from_obj[20], to_obj[20];
> struct strbuf **split;
> int err;
> @@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char
> *rewrite_cmd)
> if (!split[0] || !split[1])
> die(_("Malformed input line: '%s'."), buf.buf);
> strbuf_rtrim(split[0]);
> - strbuf_rtrim(split[1]);
> if (get_sha1(split[0]->buf, from_obj))
> die(_("Failed to resolve '%s' as a valid ref."),
> split[0]->buf);
> if (get_sha1(split[1]->buf, to_obj))
> --
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line
Jacob Keller writes:
> +static int sanitize_submodule_config(const char *var, const char *value,
> void *data)
> +{
> + struct strbuf quoted = STRBUF_INIT;
> + struct strbuf *out = data;
> +
> + if (submodule_config_ok(var)) {
> + if (out->len)
> + strbuf_addch(out, ' ');
> +
> + sq_quotef(out, "%s=%s", var, value);
Can a configuration variable that comes from the original command
line be a boolean true that is spelled without "=true", i.e. can
value be NULL here?
> + }
> +
> + strbuf_release("ed);
> +
> + return 0;
> +}
> +
> +static void prepare_submodule_repo_env(struct argv_array *out)
> +{
> + const char * const *var;
> +
> + for (var = local_repo_env; *var; var++) {
> + if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> + struct strbuf sanitized_config = STRBUF_INIT;
> + git_config_from_parameters(sanitize_submodule_config,
> +&sanitized_config);
> + argv_array_pushf(out, "%s=%s", *var,
> sanitized_config.buf);
> + strbuf_release(&sanitized_config);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/22] builtin/blame.c: mark strings for translation
Duy Nguyen writes: > On Mon, Feb 29, 2016 at 1:57 AM, Junio C Hamano wrote: >> Nguyễn Thái Ngọc Duy writes: >> >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>> builtin/blame.c | 58 >>> - >>> 1 file changed, 29 insertions(+), 29 deletions(-) >> >> I think most of the strings we see here are not new ones introduced >> in this cycle. I doubt it is a good idea to disturb the codebase, >> distract ourselves and adding last-minute workload to translators >> with this during the pre-release period. > > Yes, it's ok to consider this series a new topic, to be graduated after 2.8.0. I wasn't talking about the whole topic, but 02/22 did not look suitable for the pre-release fix. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/22] builtin/config.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
This does look like a "new i18n bug" introduced in this cycle, but
given that this program does not have much _() in the first place,
I'm inclined to say we should do the whole thing post 2.8.0 release
for this file, discarding this patch.
> builtin/config.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index ca9f834..98ca43d 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -378,7 +378,7 @@ static int get_colorbool(const char *var, int print)
> static void check_write(void)
> {
> if (!given_config_source.file && !startup_info->have_repository)
> - die("not in a git directory");
> + die(_("not in a git directory"));
>
> if (given_config_source.use_stdin)
> die("writing to stdin is not supported");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/22] builtin/update-index.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
This does look like a "new i18n bug" introduced in this cycle, but
given that this program does not have many _()s in the first place,
I'm inclined to say we should do the whole thing post 2.8.0 release
for this file, discarding this patch.
> builtin/update-index.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 1c94ca5..21e38a8 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1127,9 +1127,9 @@ int cmd_update_index(int argc, const char **argv, const
> char *prefix)
> break;
> case UC_DISABLE:
> if (git_config_get_untracked_cache() == 1)
> - warning("core.untrackedCache is set to true; "
> - "remove or change it, if you really want to "
> - "disable the untracked cache");
> + warning(_("core.untrackedCache is set to true; "
> + "remove or change it, if you really want to "
> + "disable the untracked cache"));
> remove_untracked_cache(&the_index);
> report(_("Untracked cache disabled"));
> break;
> @@ -1139,9 +1139,9 @@ int cmd_update_index(int argc, const char **argv, const
> char *prefix)
> case UC_ENABLE:
> case UC_FORCE:
> if (git_config_get_untracked_cache() == 0)
> - warning("core.untrackedCache is set to false; "
> - "remove or change it, if you really want to "
> - "enable the untracked cache");
> + warning(_("core.untrackedCache is set to false; "
> + "remove or change it, if you really want to "
> + "enable the untracked cache"));
> add_untracked_cache(&the_index);
> report(_("Untracked cache enabled for '%s'"),
> get_git_work_tree());
> break;
> @@ -1156,7 +1156,7 @@ int cmd_update_index(int argc, const char **argv, const
> char *prefix)
> unable_to_lock_die(get_index_file(), lock_error);
> }
> if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
> - die("Unable to write new index file");
> + die(_("Unable to write new index file"));
> }
>
> rollback_lock_file(lock_file);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/22] convert.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most of) these look old ones. I'm inclined to say
we should do the whole thing post 2.8.0 release for this file.
> convert.c | 30 +-
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index f524b8d..59d03b0 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -199,9 +199,11 @@ static void check_safe_crlf(const char *path, enum
> crlf_action crlf_action,
>*/
> if (stats->crlf) {
> if (checksafe == SAFE_CRLF_WARN)
> - warning("CRLF will be replaced by LF in
> %s.\nThe file will have its original line endings in your working
> directory.", path);
> + warning(_("CRLF will be replaced by LF in %s.\n"
> + "The file will have its original line
> "
> + "endings in your working
> directory."), path);
> else /* i.e. SAFE_CRLF_FAIL */
> - die("CRLF would be replaced by LF in %s.",
> path);
> + die(_("CRLF would be replaced by LF in %s."),
> path);
> }
> } else if (output_eol(crlf_action) == EOL_CRLF) {
> /*
> @@ -210,9 +212,11 @@ static void check_safe_crlf(const char *path, enum
> crlf_action crlf_action,
>*/
> if (stats->lonelf) {
> if (checksafe == SAFE_CRLF_WARN)
> - warning("LF will be replaced by CRLF in
> %s.\nThe file will have its original line endings in your working
> directory.", path);
> + warning(_("LF will be replaced by CRLF in %s.\n"
> + "The file will have its original line
> "
> + "endings in your working
> directory."), path);
> else /* i.e. SAFE_CRLF_FAIL */
> - die("LF would be replaced by CRLF in %s", path);
> + die(_("LF would be replaced by CRLF in %s"),
> path);
> }
> }
> }
> @@ -397,7 +401,7 @@ static int filter_buffer_or_fd(int in, int out, void
> *data)
> child_process.out = out;
>
> if (start_command(&child_process))
> - return error("cannot fork to run external filter %s",
> params->cmd);
> + return error(_("cannot fork to run external filter %s"),
> params->cmd);
>
> sigchain_push(SIGPIPE, SIG_IGN);
>
> @@ -415,13 +419,13 @@ static int filter_buffer_or_fd(int in, int out, void
> *data)
> if (close(child_process.in))
> write_err = 1;
> if (write_err)
> - error("cannot feed the input to external filter %s",
> params->cmd);
> + error(_("cannot feed the input to external filter %s"),
> params->cmd);
>
> sigchain_pop(SIGPIPE);
>
> status = finish_command(&child_process);
> if (status)
> - error("external filter %s failed %d", params->cmd, status);
> + error(_("external filter %s failed %d"), params->cmd, status);
>
> strbuf_release(&cmd);
> return (write_err || status);
> @@ -462,15 +466,15 @@ static int apply_filter(const char *path, const char
> *src, size_t len, int fd,
> return 0; /* error was already reported */
>
> if (strbuf_read(&nbuf, async.out, len) < 0) {
> - error("read from external filter %s failed", cmd);
> + error(_("read from external filter %s failed"), cmd);
> ret = 0;
> }
> if (close(async.out)) {
> - error("read from external filter %s failed", cmd);
> + error(_("read from external filter %s failed"), cmd);
> ret = 0;
> }
> if (finish_async(&async)) {
> - error("external filter %s failed", cmd);
> + error(_("external filter %s failed"), cmd);
> ret = 0;
> }
>
> @@ -868,7 +872,7 @@ int convert_to_git(const char *path, const char *src,
> size_t len,
>
> ret |= apply_filter(path, src, len, -1, dst, filter);
> if (!ret && required)
> - die("%s: clean filter '%s' failed", path, ca.drv->name);
> + die(_("%s: clean filter '%s' failed"), path, ca.drv->name);
>
> if (ret && dst) {
> src = dst->buf;
> @@ -892,7 +896,7 @@ void convert_to_git_filter_fd(const char *path, int fd,
> struct strbuf *dst,
> assert(ca.drv->clean);
>
> if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
> - die("%s: clean filter '%s' failed", path, ca.drv->name);
> + die(_("%s: clean filter '%s' failed"), path, ca.drv->name);
>
> crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
> ident_to_git(
Re: [PATCH 09/22] credential-cache--daemon.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most of) these look old ones. I'm inclined to say
we should do the whole thing post 2.8.0 release for this file.
> credential-cache--daemon.c | 34 +-
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
> index 63ca7c8..ab33355 100644
> --- a/credential-cache--daemon.c
> +++ b/credential-cache--daemon.c
> @@ -98,12 +98,12 @@ static int read_request(FILE *fh, struct credential *c,
>
> strbuf_getline_lf(&item, fh);
> if (!skip_prefix(item.buf, "action=", &p))
> - return error("client sent bogus action line: %s", item.buf);
> + return error(_("client sent bogus action line: %s"), item.buf);
> strbuf_addstr(action, p);
>
> strbuf_getline_lf(&item, fh);
> if (!skip_prefix(item.buf, "timeout=", &p))
> - return error("client sent bogus timeout line: %s", item.buf);
> + return error(_("client sent bogus timeout line: %s"), item.buf);
> *timeout = atoi(p);
>
> if (credential_read(c, fh) < 0)
> @@ -132,16 +132,16 @@ static void serve_one_client(FILE *in, FILE *out)
> remove_credential(&c);
> else if (!strcmp(action.buf, "store")) {
> if (timeout < 0)
> - warning("cache client didn't specify a timeout");
> + warning(_("cache client didn't specify a timeout"));
> else if (!c.username || !c.password)
> - warning("cache client gave us a partial credential");
> + warning(_("cache client gave us a partial credential"));
> else {
> remove_credential(&c);
> cache_credential(&c, timeout);
> }
> }
> else
> - warning("cache client sent unknown action: %s", action.buf);
> + warning(_("cache client sent unknown action: %s"), action.buf);
>
> credential_clear(&c);
> strbuf_release(&action);
> @@ -160,7 +160,7 @@ static int serve_cache_loop(int fd)
> pfd.events = POLLIN;
> if (poll(&pfd, 1, 1000 * wakeup) < 0) {
> if (errno != EINTR)
> - die_errno("poll failed");
> + die_errno(_("poll failed"));
> return 1;
> }
>
> @@ -170,12 +170,12 @@ static int serve_cache_loop(int fd)
>
> client = accept(fd, NULL, NULL);
> if (client < 0) {
> - warning("accept failed: %s", strerror(errno));
> + warning(_("accept failed: %s"), strerror(errno));
> return 1;
> }
> client2 = dup(client);
> if (client2 < 0) {
> - warning("dup failed: %s", strerror(errno));
> + warning(_("dup failed: %s"), strerror(errno));
> close(client);
> return 1;
> }
> @@ -195,13 +195,13 @@ static void serve_cache(const char *socket_path, int
> debug)
>
> fd = unix_stream_listen(socket_path);
> if (fd < 0)
> - die_errno("unable to bind to '%s'", socket_path);
> + die_errno(_("unable to bind to '%s'"), socket_path);
>
> printf("ok\n");
> fclose(stdout);
> if (!debug) {
> if (!freopen("/dev/null", "w", stderr))
> - die_errno("unable to point stderr to /dev/null");
> + die_errno(_("unable to point stderr to /dev/null"));
> }
>
> while (serve_cache_loop(fd))
> @@ -211,10 +211,10 @@ static void serve_cache(const char *socket_path, int
> debug)
> }
>
> static const char permissions_advice[] =
> -"The permissions on your socket directory are too loose; other\n"
> +N_("The permissions on your socket directory are too loose; other\n"
> "users may be able to read your cached credentials. Consider running:\n"
> "\n"
> -"chmod 0700 %s";
> +"chmod 0700 %s");
> static void init_socket_directory(const char *path)
> {
> struct stat st;
> @@ -223,7 +223,7 @@ static void init_socket_directory(const char *path)
>
> if (!stat(dir, &st)) {
> if (st.st_mode & 077)
> - die(permissions_advice, dir);
> + die(_(permissions_advice), dir);
> } else {
> /*
>* We must be sure to create the directory with the correct
> mode,
> @@ -232,9 +232,9 @@ static void init_socket_directory(const char *path)
>* our protected socket.
>*/
> if (safe_create_leading_directories_const(dir) < 0)
> - die_errno("unable to create directories for '%s'", dir);
> + die_errno(_("unable to create directories for '%s'"),
> dir);
> if (m
Re: [PATCH 10/22] http.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most of) these look old ones. I'm inclined to say
we should do the whole thing post 2.8.0 release for this file.
> http.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/http.c b/http.c
> index 1d5e3bb..d56cb52 100644
> --- a/http.c
> +++ b/http.c
> @@ -404,7 +404,7 @@ static void init_curl_proxy_auth(CURL *result)
> }
> }
> if (i == ARRAY_SIZE(proxy_authmethods)) {
> - warning("unsupported proxy authentication method %s:
> using anyauth",
> + warning(_("unsupported proxy authentication method %s:
> using anyauth"),
> http_proxy_authmethod);
> curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> CURLAUTH_ANY);
> }
> @@ -445,7 +445,7 @@ static int sockopt_callback(void *client, curl_socket_t
> fd, curlsocktype type)
>
> rc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len);
> if (rc < 0)
> - warning("unable to set SO_KEEPALIVE on socket %s",
> + warning(_("unable to set SO_KEEPALIVE on socket %s"),
> strerror(errno));
>
> return 0; /* CURL_SOCKOPT_OK only exists since curl 7.21.5 */
> @@ -469,7 +469,7 @@ static CURL *get_curl_handle(void)
> long allowed_protocols = 0;
>
> if (!result)
> - die("curl_easy_init failed");
> + die(_("curl_easy_init failed"));
>
> if (!curl_ssl_verify) {
> curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
> @@ -503,7 +503,7 @@ static CURL *get_curl_handle(void)
> }
> }
> if (i == ARRAY_SIZE(sslversions))
> - warning("unsupported ssl version %s: using default",
> + warning(_("unsupported ssl version %s: using default"),
> ssl_version);
> }
>
> @@ -558,8 +558,8 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> #else
> if (transport_restrict_protocols())
> - warning("protocol restrictions not applied to curl redirects
> because\n"
> - "your curl version is too old (>= 7.19.4)");
> + warning(_("protocol restrictions not applied to curl redirects
> because\n"
> + "your curl version is too old (>= 7.19.4)"));
> #endif
>
> if (getenv("GIT_CURL_VERBOSE"))
> @@ -659,7 +659,7 @@ void http_init(struct remote *remote, const char *url,
> int proactive_auth)
> free(normalized_url);
>
> if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
> - die("curl_global_init failed");
> + die(_("curl_global_init failed"));
>
> http_proactive_auth = proactive_auth;
>
> @@ -681,7 +681,7 @@ void http_init(struct remote *remote, const char *url,
> int proactive_auth)
>
> curlm = curl_multi_init();
> if (!curlm)
> - die("curl_multi_init failed");
> + die(_("curl_multi_init failed"));
> #endif
>
> if (getenv("GIT_SSL_NO_VERIFY"))
> @@ -1544,7 +1544,7 @@ static int http_get_file(const char *url, const char
> *filename,
> strbuf_addf(&tmpfile, "%s.temp", filename);
> result = fopen(tmpfile.buf, "a");
> if (!result) {
> - error("Unable to open local file %s", tmpfile.buf);
> + error(_("Unable to open local file %s"), tmpfile.buf);
> ret = HTTP_ERROR;
> goto cleanup;
> }
> @@ -1601,7 +1601,7 @@ static char *fetch_pack_index(unsigned char *sha1,
> const char *base_url)
> tmp = strbuf_detach(&buf, NULL);
>
> if (http_get_file(url, tmp, NULL) != HTTP_OK) {
> - error("Unable to get pack index %s", url);
> + error(_("Unable to get pack index %s"), url);
> free(tmp);
> tmp = NULL;
> }
> @@ -1778,7 +1778,7 @@ struct http_pack_request *new_http_pack_request(
> sha1_pack_name(target->sha1));
> preq->packfile = fopen(preq->tmpfile, "a");
> if (!preq->packfile) {
> - error("Unable to open local file %s for pack",
> + error(_("Unable to open local file %s for pack"),
> preq->tmpfile);
> goto abort;
> }
> @@ -1866,7 +1866,7 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
> unlink_or_warn(freq->tmpfile);
>
> if (freq->localfile != -1)
> - error("fd leakage in start: %d", freq->localfile);
> + error(_("fd leakage in start: %d"), freq->localfile);
> freq->localfile = open(freq->tmpfile,
> O_WRONLY | O_CREAT | O_EXCL, 0666);
> /*
> @@ -1885,7 +1885,7 @@ struct http_objec
Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()
On Mon, Feb 29, 2016 at 3:30 AM, Moritz Neeb wrote: > Although I was not sure [4], I decided to roll out v4, in the hope that the > next > reviewers will profit by the more polished commit messages and order. > > Changes since v3 [3] (the changes to single patches are indicated below): > > * Commit messages refined > * Order of patch 4 and 5 in v2 was switched. Thanks. With the exception of my commentary on patch 4/7, I think v4 addresses all my v3 review comments. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/22] ident.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most of) these look old ones; even the ones blamed
to 59f92959 (fmt_ident: refactor strictness checks, 2016-02-04) had
original in the same file without _().
I'm inclined to say we should do the whole thing post 2.8.0 release
for this file.
> ident.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index 6e12582..367a5dc 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -75,13 +75,13 @@ static int add_mailname_host(struct strbuf *buf)
> mailname = fopen("/etc/mailname", "r");
> if (!mailname) {
> if (errno != ENOENT)
> - warning("cannot open /etc/mailname: %s",
> + warning(_("cannot open /etc/mailname: %s"),
> strerror(errno));
> return -1;
> }
> if (strbuf_getline(&mailnamebuf, mailname) == EOF) {
> if (ferror(mailname))
> - warning("cannot read /etc/mailname: %s",
> + warning(_("cannot read /etc/mailname: %s"),
> strerror(errno));
> strbuf_release(&mailnamebuf);
> fclose(mailname);
> @@ -125,7 +125,7 @@ static void add_domainname(struct strbuf *out, int
> *is_bogus)
> char buf[1024];
>
> if (gethostname(buf, sizeof(buf))) {
> - warning("cannot get host name: %s", strerror(errno));
> + warning(_("cannot get host name: %s"), strerror(errno));
> strbuf_addstr(out, "(none)");
> *is_bogus = 1;
> return;
> @@ -355,18 +355,18 @@ const char *fmt_ident(const char *name, const char
> *email,
> using_default = 1;
> if (strict && default_name_is_bogus) {
> fputs(env_hint, stderr);
> - die("unable to auto-detect name (got '%s')",
> name);
> + die(_("unable to auto-detect name (got '%s')"),
> name);
> }
> if (strict && ident_use_config_only
> && !(ident_config_given & IDENT_NAME_GIVEN))
> - die("user.useConfigOnly set but no name given");
> + die(_("user.useConfigOnly set but no name
> given"));
> }
> if (!*name) {
> struct passwd *pw;
> if (strict) {
> if (using_default)
> fputs(env_hint, stderr);
> - die("empty ident name (for <%s>) not allowed",
> email);
> + die(_("empty ident name (for <%s>) not
> allowed"), email);
> }
> pw = xgetpwuid_self(NULL);
> name = pw->pw_name;
> @@ -377,11 +377,11 @@ const char *fmt_ident(const char *name, const char
> *email,
> email = ident_default_email();
> if (strict && default_email_is_bogus) {
> fputs(env_hint, stderr);
> - die("unable to auto-detect email address (got '%s')",
> email);
> + die(_("unable to auto-detect email address (got
> '%s')"), email);
> }
> if (strict && ident_use_config_only
> && !(ident_config_given & IDENT_MAIL_GIVEN))
> - die("user.useConfigOnly set but no mail given");
> + die(_("user.useConfigOnly set but no mail given"));
> }
>
> strbuf_reset(&ident);
> @@ -396,7 +396,7 @@ const char *fmt_ident(const char *name, const char *email,
> strbuf_addch(&ident, ' ');
> if (date_str && date_str[0]) {
> if (parse_date(date_str, &ident) < 0)
> - die("invalid date format: %s", date_str);
> + die(_("invalid date format: %s"), date_str);
> }
> else
> strbuf_addstr(&ident, ident_default_date());
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/22] notes.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most of) these look old ones. I'm inclined to say
we should do the whole thing post 2.8.0 release for this file.
> notes.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/notes.c b/notes.c
> index 88cf474..0f03f77 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -422,7 +422,7 @@ static void load_subtree(struct notes_tree *t, struct
> leaf_node *subtree,
>
> buf = fill_tree_descriptor(&desc, subtree->val_sha1);
> if (!buf)
> - die("Could not read %s for notes-index",
> + die(_("Could not read %s for notes-index"),
>sha1_to_hex(subtree->val_sha1));
>
> prefix_len = subtree->key_sha1[19];
> @@ -455,8 +455,8 @@ static void load_subtree(struct notes_tree *t, struct
> leaf_node *subtree,
> }
> if (note_tree_insert(t, node, n, l, type,
>combine_notes_concatenate))
> - die("Failed to load %s %s into notes tree "
> - "from %s",
> + die(_("Failed to load %s %s into notes tree "
> + "from %s"),
> type == PTR_TYPE_NOTE ? "note" : "subtree",
> sha1_to_hex(l->key_sha1), t->ref);
> }
> @@ -942,7 +942,7 @@ void string_list_add_refs_by_glob(struct string_list
> *list, const char *glob)
> } else {
> unsigned char sha1[20];
> if (get_sha1(glob, sha1))
> - warning("notes ref %s is invalid", glob);
> + warning(_("notes ref %s is invalid"), glob);
> if (!unsorted_string_list_has_string(list, glob))
> string_list_append(list, glob);
> }
> @@ -1020,9 +1020,9 @@ void init_notes(struct notes_tree *t, const char
> *notes_ref,
> get_sha1_treeish(notes_ref, object_sha1))
> return;
> if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
> - die("Cannot use notes ref %s", notes_ref);
> + die(_("Cannot use notes ref %s"), notes_ref);
> if (get_tree_entry(object_sha1, "", sha1, &mode))
> - die("Failed to read notes tree referenced by %s (%s)",
> + die(_("Failed to read notes tree referenced by %s (%s)"),
> notes_ref, sha1_to_hex(object_sha1));
>
> hashclr(root_tree.key_sha1);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/22] ref-filter.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
About half of this we can find in 2.7.0, but we can see that many
existing messages are already marked. Let's take this for 2.8.0 (I
am assuming that this covers the whole file, not just relatively new
ones).
> ref-filter.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index bb79d6b..bc551a7 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -74,14 +74,14 @@ static void remote_ref_atom_parser(struct used_atom
> *atom, const char *arg)
> static void body_atom_parser(struct used_atom *atom, const char *arg)
> {
> if (arg)
> - die("%%(body) does not take arguments");
> + die(_("%%(body) does not take arguments"));
> atom->u.contents.option = C_BODY_DEP;
> }
>
> static void subject_atom_parser(struct used_atom *atom, const char *arg)
> {
> if (arg)
> - die("%%(subject) does not take arguments");
> + die(_("%%(subject) does not take arguments"));
> atom->u.contents.option = C_SUB;
> }
>
> @@ -241,7 +241,7 @@ int parse_ref_filter_atom(const char *atom, const char
> *ep)
> if (*sp == '*' && sp < ep)
> sp++; /* deref */
> if (ep <= sp)
> - die("malformed field name: %.*s", (int)(ep-atom), atom);
> + die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
>
> /* Do we have the atom already used elsewhere? */
> for (i = 0; i < used_atom_cnt; i++) {
> @@ -267,7 +267,7 @@ int parse_ref_filter_atom(const char *atom, const char
> *ep)
> }
>
> if (ARRAY_SIZE(valid_atom) <= i)
> - die("unknown field name: %.*s", (int)(ep-atom), atom);
> + die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
>
> /* Add it in, including the deref prefix */
> at = used_atom_cnt;
> @@ -421,7 +421,7 @@ int verify_ref_format(const char *format)
> int at;
>
> if (!ep)
> - return error("malformed format string %s", sp);
> + return error(_("malformed format string %s"), sp);
> /* sp points at "%(" and ep points at the closing ")" */
> at = parse_ref_filter_atom(sp + 2, ep);
> cp = ep + 1;
> @@ -875,12 +875,12 @@ static const char *strip_ref_components(const char
> *refname, const char *nr_arg)
> const char *start = refname;
>
> if (nr < 1 || *end != '\0')
> - die(":strip= requires a positive integer argument");
> + die(_(":strip= requires a positive integer argument"));
>
> while (remaining) {
> switch (*start++) {
> case '\0':
> - die("ref '%s' does not have %ld components to :strip",
> + die(_("ref '%s' does not have %ld components to
> :strip"),
> refname, nr);
> case '/':
> remaining--;
> @@ -1043,7 +1043,7 @@ static void populate_value(struct ref_array_item *ref)
> else if (skip_prefix(formatp, "strip=", &arg))
> refname = strip_ref_components(refname, arg);
> else
> - die("unknown %.*s format %s",
> + die(_("unknown %.*s format %s"),
> (int)(formatp - name), name, formatp);
> }
>
> @@ -1063,10 +1063,10 @@ static void populate_value(struct ref_array_item *ref)
> need_obj:
> buf = get_obj(ref->objectname, &obj, &size, &eaten);
> if (!buf)
> - die("missing object %s for %s",
> + die(_("missing object %s for %s"),
> sha1_to_hex(ref->objectname), ref->refname);
> if (!obj)
> - die("parse_object_buffer failed on %s for %s",
> + die(_("parse_object_buffer failed on %s for %s"),
> sha1_to_hex(ref->objectname), ref->refname);
>
> grab_values(ref->value, 0, obj, buf, size);
> @@ -1094,10 +1094,10 @@ static void populate_value(struct ref_array_item *ref)
>*/
> buf = get_obj(tagged, &obj, &size, &eaten);
> if (!buf)
> - die("missing object %s for %s",
> + die(_("missing object %s for %s"),
> sha1_to_hex(tagged), ref->refname);
> if (!obj)
> - die("parse_object_buffer failed on %s for %s",
> + die(_("parse_object_buffer failed on %s for %s"),
> sha1_to_hex(tagged), ref->refname);
> grab_values(ref->value, 1, obj, buf, size);
> if (!eaten)
> @@ -1370,12 +1370,12 @@ static int ref_filter_handler(const char *refname,
> const struct object_id *oid,
> unsigned int kind;
>
> if (flag & REF_BAD_NAME) {
> - warning("ignoring ref with broken name %s", refname);
> +
Re: [PATCH 14/22] refs/files-backend.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
I'd really prefer to avoid any code churn on this file before the
dust settles for David's and Michael's series (the former is in
'pu', the latter is not but was already rerolled once) both of which
touch this file heavily.
> refs/files-backend.c | 86
> ++--
> 1 file changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 81f68f8..bf76094 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -203,7 +203,7 @@ static struct ref_entry *create_ref_entry(const char
> *refname,
>
> if (check_name &&
> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> - die("Reference has invalid format: '%s'", refname);
> + die(_("Reference has invalid format: '%s'"), refname);
> FLEX_ALLOC_STR(ref, name, refname);
> hashcpy(ref->u.value.oid.hash, sha1);
> oidclr(&ref->u.value.peeled);
> @@ -475,12 +475,12 @@ static int is_dup_ref(const struct ref_entry *ref1,
> const struct ref_entry *ref2
>
> if ((ref1->flag & REF_DIR) || (ref2->flag & REF_DIR))
> /* This is impossible by construction */
> - die("Reference directory conflict: %s", ref1->name);
> + die(_("Reference directory conflict: %s"), ref1->name);
>
> if (oidcmp(&ref1->u.value.oid, &ref2->u.value.oid))
> - die("Duplicated ref, and SHA1s don't match: %s", ref1->name);
> + die(_("Duplicated ref, and SHA1s don't match: %s"), ref1->name);
>
> - warning("Duplicated ref: %s", ref1->name);
> + warning(_("Duplicated ref: %s"), ref1->name);
> return 1;
> }
>
> @@ -526,7 +526,7 @@ static int ref_resolves_to_object(struct ref_entry *entry)
> if (entry->flag & REF_ISBROKEN)
> return 0;
> if (!has_sha1_file(entry->u.value.oid.hash)) {
> - error("%s does not point to a valid object!", entry->name);
> + error(_("%s does not point to a valid object!"), entry->name);
> return 0;
> }
> return 1;
> @@ -653,7 +653,7 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
> i1++;
> i2++;
> } else {
> - die("conflict between reference and directory:
> %s",
> + die(_("conflict between reference and
> directory: %s"),
> e1->name);
> }
> } else {
> @@ -914,7 +914,7 @@ static void clear_packed_ref_cache(struct ref_cache *refs)
> struct packed_ref_cache *packed_refs = refs->packed;
>
> if (packed_refs->lock)
> - die("internal error: packed-ref cache cleared while
> locked");
> + die(_("internal error: packed-ref cache cleared while
> locked"));
> refs->packed = NULL;
> release_packed_ref_cache(packed_refs);
> }
> @@ -1069,7 +1069,7 @@ static void read_packed_refs(FILE *f, struct ref_dir
> *dir)
>
> if (check_refname_format(refname,
> REFNAME_ALLOW_ONELEVEL)) {
> if (!refname_is_safe(refname))
> - die("packed refname is dangerous: %s",
> refname);
> + die(_("packed refname is dangerous:
> %s"), refname);
> hashclr(sha1);
> flag |= REF_BAD_NAME | REF_ISBROKEN;
> }
> @@ -1239,7 +1239,7 @@ static void read_loose_refs(const char *dirname, struct
> ref_dir *dir)
> if (check_refname_format(refname.buf,
>REFNAME_ALLOW_ONELEVEL)) {
> if (!refname_is_safe(refname.buf))
> - die("loose refname is dangerous: %s",
> refname.buf);
> + die(_("loose refname is dangerous:
> %s"), refname.buf);
> hashclr(sha1);
> flag |= REF_BAD_NAME | REF_ISBROKEN;
> }
> @@ -2099,7 +2099,7 @@ static int commit_packed_refs(void)
>
> out = fdopen_lock_file(packed_ref_cache->lock, "w");
> if (!out)
> - die_errno("unable to fdopen packed-refs descriptor");
> + die_errno(_("unable to fdopen packed-refs descriptor"));
>
> fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
> do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
> @@ -2275,7 +2275,7 @@ int pack_refs(unsigned int flags)
>pack_if_possible_fn, &cbdata);
>
> if (commit_packed_refs())
> - die_errno("unable to overwrite old ref-pack file");
> + die_e
Re: [PATCH 15/22] remote-curl.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
All (or at least most) of these look old ones. I'm inclined to say
we should do the whole thing post 2.8.0 release for this file.
> remote-curl.c | 40
> 1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..d5b33aa 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -177,7 +177,7 @@ static struct ref *parse_info_refs(struct discovery
> *heads)
> mid = &data[i];
> if (data[i] == '\n') {
> if (mid - start != 40)
> - die("%sinfo/refs not valid: is this a git
> repository?",
> + die(_("%sinfo/refs not valid: is this a git
> repository?"),
> url.buf);
> data[i] = 0;
> ref_name = mid + 1;
> @@ -285,13 +285,13 @@ static struct discovery *discover_refs(const char
> *service, int for_push)
> break;
> case HTTP_MISSING_TARGET:
> show_http_message(&type, &charset, &buffer);
> - die("repository '%s' not found", url.buf);
> + die(_("repository '%s' not found"), url.buf);
> case HTTP_NOAUTH:
> show_http_message(&type, &charset, &buffer);
> - die("Authentication failed for '%s'", url.buf);
> + die(_("Authentication failed for '%s'"), url.buf);
> default:
> show_http_message(&type, &charset, &buffer);
> - die("unable to access '%s': %s", url.buf, curl_errorstr);
> + die(_("unable to access '%s': %s"), url.buf, curl_errorstr);
> }
>
> last= xcalloc(1, sizeof(*last_discovery));
> @@ -314,7 +314,7 @@ static struct discovery *discover_refs(const char
> *service, int for_push)
> strbuf_reset(&exp);
> strbuf_addf(&exp, "# service=%s", service);
> if (strcmp(line, exp.buf))
> - die("invalid server response; got '%s'", line);
> + die(_("invalid server response; got '%s'"), line);
> strbuf_release(&exp);
>
> /* The header can include additional metadata lines, up
> @@ -422,7 +422,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void
> *clientp)
> rpc->pos = 0;
> return CURLIOE_OK;
> }
> - error("unable to rewind rpc post data - try increasing
> http.postBuffer");
> + error(_("unable to rewind rpc post data - try increasing
> http.postBuffer"));
> return CURLIOE_FAILRESTART;
>
> default:
> @@ -604,11 +604,11 @@ retry:
>
> ret = git_deflate(&stream, Z_FINISH);
> if (ret != Z_STREAM_END)
> - die("cannot deflate request; zlib deflate error %d",
> ret);
> + die(_("cannot deflate request; zlib deflate error %d"),
> ret);
>
> ret = git_deflate_end_gently(&stream);
> if (ret != Z_OK)
> - die("cannot deflate request; zlib end error %d", ret);
> + die(_("cannot deflate request; zlib end error %d"),
> ret);
>
> gzip_size = stream.total_out;
>
> @@ -726,7 +726,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
>
> ALLOC_ARRAY(targets, nr_heads);
> if (options.depth)
> - die("dumb http transport does not support --depth");
> + die(_("dumb http transport does not support --depth"));
> for (i = 0; i < nr_heads; i++)
> targets[i] = xstrdup(oid_to_hex(&to_fetch[i]->old_oid));
>
> @@ -743,7 +743,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
> free(targets[i]);
> free(targets);
>
> - return ret ? error("fetch failed.") : 0;
> + return ret ? error(_("fetch failed.")) : 0;
> }
>
> static int fetch_git(struct discovery *heads,
> @@ -787,7 +787,7 @@ static int fetch_git(struct discovery *heads,
> for (i = 0; i < nr_heads; i++) {
> struct ref *ref = to_fetch[i];
> if (!*ref->name)
> - die("cannot fetch by sha1 over smart http");
> + die(_("cannot fetch by sha1 over smart http"));
> packet_buf_write(&preamble, "%s %s\n",
>oid_to_hex(&ref->old_oid), ref->name);
> }
> @@ -832,13 +832,13 @@ static void parse_fetch(struct strbuf *buf)
> struct object_id old_oid;
>
> if (get_oid_hex(p, &old_oid))
> - die("protocol error: expected sha/ref, got
> %s'", p);
> + die(_("protocol error: expected sha/ref, got
> %s'"), p);
> if (p[GIT_SHA1_HEXSZ] == ' ')
>
Re: [PATCH 16/22] run-command.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
This is new and there aren't many non-BUG messages that are not
marked, so let's take this one for 2.8.0.
> run-command.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 863dad5..1ee2357 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -909,7 +909,7 @@ static int default_start_failure(struct child_process *cp,
> {
> int i;
>
> - strbuf_addstr(err, "Starting a child failed:");
> + strbuf_addstr(err, _("Starting a child failed:"));
> for (i = 0; cp->argv[i]; i++)
> strbuf_addf(err, " %s", cp->argv[i]);
>
> @@ -927,7 +927,7 @@ static int default_task_finished(int result,
> if (!result)
> return 0;
>
> - strbuf_addf(err, "A child failed with return code %d:", result);
> + strbuf_addf(err, _("A child failed with return code %d:"), result);
> for (i = 0; cp->argv[i]; i++)
> strbuf_addf(err, " %s", cp->argv[i]);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/22] trailer.c: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
I'll take this one, even though it is quite old, as the file has a
serious amount of messages that are already marked.
> trailer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/trailer.c b/trailer.c
> index 94b387b..8e48a5c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -234,7 +234,7 @@ static const char *apply_command(const char *command,
> const char *arg)
> cp.use_shell = 1;
>
> if (capture_command(&cp, &buf, 1024)) {
> - error("running trailer command '%s' failed", cmd.buf);
> + error(_("running trailer command '%s' failed"), cmd.buf);
> strbuf_release(&buf);
> result = xstrdup("");
> } else {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step
Moritz Neeb writes:
> Because sq_quote_argv adds a leading space (which is expected in trace.c),
> sq_dequote_step should remove this space again, such that the operations
> of quoting and dequoting are inverse of each other.
>
> This patch is preparing the way to remove some excessive trimming
> operation in bisect in the following commit.
>
> Signed-off-by: Moritz Neeb
> ---
> quote.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/quote.c b/quote.c
> index fe884d2..2714f27 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
> char *src = arg;
> char c;
>
> + if (*src == ' ')
> + src++;
> if (*src != '\'')
> return NULL;
> for (;;) {
If we look at this "for (;;)" loop, we notice that (1) it accepts as
many spaces as there are between two quoted strings, and (2) it does
not limit it to SP but uses isspace().
I wonder if you would instead want
while (isspace(*src))
src++;
to be consistent?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv19 09/11] git submodule update: have a dedicated helper for cloning
On Sat, Feb 27, 2016 at 12:40 AM, Duy Nguyen wrote:
> On Fri, Feb 26, 2016 at 6:48 AM, Stefan Beller wrote:
>> +static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>> + struct child_process *child,
>> + struct submodule_update_clone
>> *suc,
>> + struct strbuf *out)
>> +{
>> + const struct submodule *sub = NULL;
>> + struct strbuf displaypath_sb = STRBUF_INIT;
>> + struct strbuf sb = STRBUF_INIT;
>> + const char *displaypath = NULL;
>> + char *url = NULL;
>> + int needs_cloning = 0;
>> +
>> + if (ce_stage(ce)) {
>> + if (suc->recursive_prefix) {
>> + strbuf_addf(out, "Skipping unmerged submodule
>> %s/%s\n",
>> + suc->recursive_prefix, ce->name);
>
> I'm pretty sure this string is for human consumption (because it's
> _()'d elsehwere in this function), please _() this string.
>
>> + } else {
>> + strbuf_addf(out, "Skipping unmerged submodule %s\n",
>> + ce->name);
>
> and this one
>
>> + }
>> + goto cleanup;
>> + }
>> +
>> + sub = submodule_from_path(null_sha1, ce->name);
>> +
>> + if (suc->recursive_prefix)
>> + displaypath = relative_path(suc->recursive_prefix,
>> + ce->name, &displaypath_sb);
>> + else
>> + displaypath = ce->name;
>> +
>> + if (suc->update.type == SM_UPDATE_NONE
>> + || (suc->update.type == SM_UPDATE_UNSPECIFIED
>> + && sub->update_strategy.type == SM_UPDATE_NONE)) {
>> + strbuf_addf(out, "Skipping submodule '%s'\n",
>> + displaypath);
>
> and this one
>
>> + goto cleanup;
>> + }
>> +
>> + /*
>> +* Looking up the url in .git/config.
>> +* We must not fall back to .gitmodules as we only want
>> +* to process configured submodules.
>> +*/
>> + strbuf_reset(&sb);
>> + strbuf_addf(&sb, "submodule.%s.url", sub->name);
>> + git_config_get_string(sb.buf, &url);
>> + if (!url) {
>> + /*
>> +* Only mention uninitialized submodules when their
>> +* path have been specified
>> +*/
>> + if (suc->warn_if_uninitialized)
>> + strbuf_addf(out, _("Submodule path '%s' not
>> initialized\n"
>> + "Maybe you want to use 'update
>> --init'?\n"),
>> + displaypath);
>
> oh it's already marked :)
>
> BTW, while you're editing this file, perhaps do this too (maybe in a
> separate patch)? Because die() already prepends "fatal:"
Makes sense. As builtin/submodule--helper.c was introduced in 2.7.0
and translation has already started fr 2.8.0, I'll just pick it up as
part of this series
instead of sending a bugfix patch alone.
Thanks for review!
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a6e54fa..6cf47de 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -731,13 +731,13 @@ int cmd_submodule__helper(int argc, const char
> **argv, const char *prefix)
> {
> int i;
> if (argc < 2)
> - die(_("fatal: submodule--helper subcommand must be "
> + die(_("submodule--helper subcommand must be "
> "called with a subcommand"));
>
> for (i = 0; i < ARRAY_SIZE(commands); i++)
> if (!strcmp(argv[1], commands[i].cmd))
> return commands[i].fn(argc - 1, argv + 1, prefix);
>
> - die(_("fatal: '%s' is not a valid submodule--helper "
> + die(_("'%s' is not a valid submodule--helper "
> "subcommand"), argv[1]);
> }
> --
> Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e
Jeff King writes: > On Mon, Feb 29, 2016 at 10:56:22AM +0100, Torsten Bögershausen wrote: > >> Thanks for the fast-patch. >> >> I manually copied the patch, But there are more probles, it seems. >> >> $ git diff >> diff --git a/compat/mingw.c b/compat/mingw.c >> index cfedcf9..b1163f2 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const >> char **argv, char **deltaen >> free(quoted); >> } >> >> - wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), >> sizeof(wchar_t)); >> + wargs = ALLOC_ARRAY(st_add(st_mult(2, args.len), 1), >> sizeof(wchar_t)); >> xutftowcs(wargs, args.buf, 2 * args.len + 1); >> strbuf_release(&args); > > Argh. Let me write "git commit -a" on the inside of my brown paper bag, > so that I actually send out the fix sitting in my working tree, not the > half-finished thing I ran "git add" on. Just to make sure that I am not confused, what you wrote below matches what I received from you two message upthread. I am assuming that it is intended that the two messages from you have the same patch, and the assignment of ALLOC_ARRAY to wargs was a bug Torsten introduced only to his tree when cutting and pasting. With that assumption, will queue this one (or the original one, which to me is the same thing). Thanks. > -- >8 -- > Subject: [PATCH] compat/mingw: brown paper bag fix for 50a6c8e > > Commit 50a6c8e (use st_add and st_mult for allocation size > computation, 2016-02-22) fixed up many xmalloc call-sites > including ones in compat/mingw.c. > > But I screwed up one of them, which was half-converted to > ALLOC_ARRAY, using a very early prototype of the function. > And I never caught it because I don't build on Windows. > > Signed-off-by: Jeff King > --- > compat/mingw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index cfedcf9..54c82ec 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1069,7 +1069,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const > char **argv, char **deltaen > free(quoted); > } > > - wargs = xmalloc_array(st_add(st_mult(2, args.len), 1), sizeof(wchar_t)); > + ALLOC_ARRAY(wargs, st_add(st_mult(2, args.len), 1)); > xutftowcs(wargs, args.buf, 2 * args.len + 1); > strbuf_release(&args); -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 07/12] run_processes_parallel: rename parameters for the callbacks
The refs code has a similar pattern of passing around 'struct strbuf *err',
which is strictly used for error reporting. This is not the case here,
as the strbuf is used to accumulate all the output (whether it is error
or not) for the user. Rename it to 'out'.
Suggested-by: Jonathan Nieder
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
run-command.c | 12 ++--
run-command.h | 14 +++---
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/run-command.c b/run-command.c
index c9b13cf..d2964e1 100644
--- a/run-command.c
+++ b/run-command.c
@@ -903,22 +903,22 @@ struct parallel_processes {
};
int default_start_failure(struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb)
{
int i;
- strbuf_addstr(err, "Starting a child failed:");
+ strbuf_addstr(out, "Starting a child failed:");
for (i = 0; cp->argv[i]; i++)
- strbuf_addf(err, " %s", cp->argv[i]);
+ strbuf_addf(out, " %s", cp->argv[i]);
return 0;
}
int default_task_finished(int result,
struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb)
{
@@ -927,9 +927,9 @@ int default_task_finished(int result,
if (!result)
return 0;
- strbuf_addf(err, "A child failed with return code %d:", result);
+ strbuf_addf(out, "A child failed with return code %d:", result);
for (i = 0; cp->argv[i]; i++)
- strbuf_addf(err, " %s", cp->argv[i]);
+ strbuf_addf(out, " %s", cp->argv[i]);
return 0;
}
diff --git a/run-command.h b/run-command.h
index a054fa6..2bd8fee 100644
--- a/run-command.h
+++ b/run-command.h
@@ -139,7 +139,7 @@ int in_async(void);
* return the negative signal number.
*/
typedef int (*get_next_task_fn)(struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void **pp_task_cb);
@@ -148,7 +148,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,
* a new process.
*
* You must not write to stdout or stderr in this function. Add your
- * message to the strbuf err instead, which will be printed without
+ * message to the strbuf out instead, which will be printed without
* messing up the output of the other parallel processes.
*
* pp_cb is the callback cookie as passed into run_processes_parallel,
@@ -159,7 +159,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,
* the negative signal number.
*/
typedef int (*start_failure_fn)(struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb);
@@ -168,7 +168,7 @@ typedef int (*start_failure_fn)(struct child_process *cp,
* exact command which failed.
*/
int default_start_failure(struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb);
@@ -176,7 +176,7 @@ int default_start_failure(struct child_process *cp,
* This callback is called on every child process that finished processing.
*
* You must not write to stdout or stderr in this function. Add your
- * message to the strbuf err instead, which will be printed without
+ * message to the strbuf out instead, which will be printed without
* messing up the output of the other parallel processes.
*
* pp_cb is the callback cookie as passed into run_processes_parallel,
@@ -188,7 +188,7 @@ int default_start_failure(struct child_process *cp,
*/
typedef int (*task_finished_fn)(int result,
struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb);
@@ -198,7 +198,7 @@ typedef int (*task_finished_fn)(int result,
*/
int default_task_finished(int result,
struct child_process *cp,
- struct strbuf *err,
+ struct strbuf *out,
void *pp_cb,
void *pp_task_cb);
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 00/12] Expose submodule parallelism to the user
Thanks Duy for reviewing!
I added your suggestions as amending and as a new patch.
Thanks,
Stefan
Interdiff to v19 (current origin/sb/submodule-parallel-update):
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0272c98..1b0b13a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -299,10 +299,10 @@ static int prepare_to_clone_next_submodule(const struct
cache_entry *ce,
if (ce_stage(ce)) {
if (suc->recursive_prefix) {
- strbuf_addf(out, "Skipping unmerged submodule %s/%s\n",
+ strbuf_addf(out,_("Skipping unmerged submodule
%s/%s\n"),
suc->recursive_prefix, ce->name);
} else {
- strbuf_addf(out, "Skipping unmerged submodule %s\n",
+ strbuf_addf(out, _("Skipping unmerged submodule %s\n"),
ce->name);
}
goto cleanup;
@@ -319,7 +319,7 @@ static int prepare_to_clone_next_submodule(const struct
cache_entry *ce,
if (suc->update.type == SM_UPDATE_NONE
|| (suc->update.type == SM_UPDATE_UNSPECIFIED
&& sub->update_strategy.type == SM_UPDATE_NONE)) {
- strbuf_addf(out, "Skipping submodule '%s'\n",
+ strbuf_addf(out, _("Skipping submodule '%s'\n"),
displaypath);
goto cleanup;
}
@@ -525,13 +525,13 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
{
int i;
if (argc < 2)
- die(_("fatal: submodule--helper subcommand must be "
+ die(_("submodule--helper subcommand must be "
"called with a subcommand"));
for (i = 0; i < ARRAY_SIZE(commands); i++)
if (!strcmp(argv[1], commands[i].cmd))
return commands[i].fn(argc - 1, argv + 1, prefix);
- die(_("fatal: '%s' is not a valid submodule--helper "
+ die(_("'%s' is not a valid submodule--helper "
"subcommand"), argv[1]);
}
Stefan Beller (12):
submodule-config: keep update strategy around
submodule-config: drop check against NULL
fetching submodules: respect `submodule.fetchJobs` config option
submodule update: direct error message to stderr
run_processes_parallel: treat output of children as byte array
run-command: expose default_{start_failure, task_finished}
run_processes_parallel: rename parameters for the callbacks
run_processes_parallel: correctly terminate callbacks with an LF
git submodule update: have a dedicated helper for cloning
submodule helper: remove double 'fatal: ' prefix
submodule update: expose parallelism to the user
clone: allow an explicit argument for parallel submodule clones
Documentation/config.txt| 6 +
Documentation/git-clone.txt | 6 +-
Documentation/git-submodule.txt | 7 +-
builtin/clone.c | 19 ++-
builtin/fetch.c | 2 +-
builtin/submodule--helper.c | 259 +++-
git-submodule.sh| 56 -
run-command.c | 36 +++---
run-command.h | 29 -
strbuf.c| 6 +
strbuf.h| 6 +
submodule-config.c | 19 ++-
submodule-config.h | 2 +
submodule.c | 37 +-
submodule.h | 18 +++
t/t5526-fetch-submodules.sh | 14 +++
t/t7400-submodule-basic.sh | 4 +-
t/t7406-submodule-update.sh | 27 +
18 files changed, 480 insertions(+), 73 deletions(-)
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 12/12] clone: allow an explicit argument for parallel submodule clones
Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-clone.txt | 6 +-
builtin/clone.c | 19 +--
t/t7406-submodule-update.sh | 15 +++
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6bf000d..6db7b6d 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[-o ] [-b ] [-u ] [--reference ]
[--dissociate] [--separate-git-dir ]
[--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--]
+ [--recursive | --recurse-submodules] [--jobs ] [--]
[]
DESCRIPTION
@@ -221,6 +221,10 @@ objects from the source repository into a pack in the
cloned repository.
The result is Git repository can be separated from working
tree.
+-j ::
+--jobs ::
+ The number of submodules fetched at the same time.
+ Defaults to the `submodule.fetchJobs` option.
::
The (possibly remote) repository to clone from. See the
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..b004fb4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
static struct string_list option_config;
static struct string_list option_reference;
static int option_dissociate;
+static int max_jobs = -1;
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", &option_recursive,
N_("initialize submodules in the clone")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", &option_template, N_("template-directory"),
N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
};
-static const char *argv_submodule[] = {
- "submodule", "update", "--init", "--recursive", NULL
-};
-
static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
{
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
sha1_to_hex(sha1), "1", NULL);
- if (!err && option_recursive)
- err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+ if (!err && option_recursive) {
+ struct argv_array args = ARGV_ARRAY_INIT;
+ argv_array_pushl(&args, "submodule", "update", "--init",
"--recursive", NULL);
+
+ if (max_jobs != -1)
+ argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+ err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+ argv_array_clear(&args);
+ }
return err;
}
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in
parallel' '
grep "9 tasks" trace.out
)
'
+
+test_expect_success 'git clone passes the parallel jobs config on to
submodules' '
+ test_when_finished "rm -rf super4" &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 .
super4 &&
+ grep "7 tasks" trace.out &&
+ rm -rf super4 &&
+ git config --global submodule.fetchJobs 8 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+ grep "8 tasks" trace.out &&
+ rm -rf super4 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 .
super4 &&
+ grep "9 tasks" trace.out &&
+ rm -rf super4
+'
+
test_done
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 11/12] submodule update: expose parallelism to the user
Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.fetchJobs" setting.
By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-submodule.txt | 7 ++-
builtin/submodule--helper.c | 8 +++-
git-submodule.sh| 9 +
t/t7406-submodule-update.sh | 12
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..13adebf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
'git submodule' [--quiet] deinit [-f|--force] [--] ...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+-j ::
+--jobs ::
+ This option is only valid for the update command.
+ Clone new submodules in parallel with as many jobs.
+ Defaults to the `submodule.fetchJobs` option.
...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a111fd2..1b0b13a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -433,6 +433,7 @@ static int update_clone_task_finished(int result,
static int update_clone(int argc, const char **argv, const char *prefix)
{
const char *update = NULL;
+ int max_jobs = -1;
struct string_list_item *item;
struct pathspec pathspec;
struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
@@ -453,6 +454,8 @@ static int update_clone(int argc, const char **argv, const
char *prefix)
OPT_STRING(0, "depth", &suc.depth, "",
N_("Create a shallow clone truncated to the "
"specified number of revisions")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("parallel jobs")),
OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
OPT_END()
};
@@ -480,7 +483,10 @@ static int update_clone(int argc, const char **argv, const
char *prefix)
gitmodules_config();
git_config(submodule_config, NULL);
- run_processes_parallel(1,
+ if (max_jobs < 0)
+ max_jobs = parallel_submodules();
+
+ run_processes_parallel(max_jobs,
update_clone_get_next_task,
update_clone_start_failure,
update_clone_task_finished,
diff --git a/git-submodule.sh b/git-submodule.sh
index a6a82d2..86018ee 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -j|--jobs)
+ case "$2" in '') usage ;; esac
+ jobs="--jobs=$2"
+ shift
+ ;;
+ --jobs=*)
+ jobs=$1
+ ;;
--)
shift
break
@@ -671,6 +679,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+ ${jobs:+$jobs} \
"$@" || echo "#unmatched"
} | {
err=
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops
module name before recur
test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked
out" actual
)
'
+
+test_expect_success 'submodule update can be run in parallel' '
+ (cd super2 &&
+GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&
+grep "7 tasks" trace.out &&
+git config submodule.fetchJobs 8 &&
+GIT_TRACE=$(pwd)/trace.out git submodule update &&
+grep "8 tasks" trace.out &&
+GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&
+grep "9 tasks" trace.out
+
[PATCHv20 03/12] fetching submodules: respect `submodule.fetchJobs` config option
This allows to configure fetching and updating in parallel
without having the command line option.
This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default).
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/config.txt| 6 ++
builtin/fetch.c | 2 +-
submodule.c | 16 +++-
submodule.h | 2 ++
t/t5526-fetch-submodules.sh | 14 ++
5 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2646,6 +2646,12 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+submodule.fetchJobs::
+ Specifies how many submodules are fetched/cloned at the same time.
+ A positive integer allows up to that number of submodules fetched
+ in parallel. A value of 0 will give some reasonable default.
+ If unset, it defaults to 1.
+
tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..5aa1c2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
static int all, append, dry_run, force, keep, multiple, update_head_ok,
verbosity;
static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
static const char *depth;
static const char *upload_pack;
static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule.c b/submodule.c
index b38dd51..051f722 100644
--- a/submodule.c
+++ b/submodule.c
@@ -15,6 +15,7 @@
#include "thread-utils.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int parallel_jobs = 1;
static struct string_list changed_submodule_paths;
static int initialized_fetch_ref_tips;
static struct sha1_array ref_tips_before_fetch;
@@ -169,7 +170,12 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
int submodule_config(const char *var, const char *value, void *cb)
{
- if (starts_with(var, "submodule."))
+ if (!strcmp(var, "submodule.fetchjobs")) {
+ parallel_jobs = git_config_int(var, value);
+ if (parallel_jobs < 0)
+ die(_("negative values not allowed for
submodule.fetchJobs"));
+ return 0;
+ } else if (starts_with(var, "submodule."))
return parse_submodule_config_option(var, value);
else if (!strcmp(var, "fetch.recursesubmodules")) {
config_fetch_recurse_submodules =
parse_fetch_recurse_submodules_arg(var, value);
@@ -772,6 +778,9 @@ int fetch_populated_submodules(const struct argv_array
*options,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
+ if (max_parallel_jobs < 0)
+ max_parallel_jobs = parallel_jobs;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
@@ -1118,3 +1127,8 @@ void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir)
strbuf_release(&rel_path);
free((void *)real_work_tree);
}
+
+int parallel_submodules(void)
+{
+ return parallel_jobs;
+}
diff --git a/submodule.h b/submodule.h
index 3464500..3166608 100644
--- a/submodule.h
+++ b/submodule.h
@@ -26,6 +26,7 @@ struct submodule_update_strategy {
enum submodule_update_type type;
const char *command;
};
+#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
@@ -57,5 +58,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_nam
struct string_list *needs_pushing);
int push_unpushed_submodules(unsigned char new_sha1[20], const char
*remotes_name);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+int parallel_submodules(void);
#endif
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1241146..954d0e4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when ne
[PATCHv20 10/12] submodule helper: remove double 'fatal: ' prefix
The prefix is added by die(...), so we don't have to do it.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e1b60ff..a111fd2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -519,13 +519,13 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
{
int i;
if (argc < 2)
- die(_("fatal: submodule--helper subcommand must be "
+ die(_("submodule--helper subcommand must be "
"called with a subcommand"));
for (i = 0; i < ARRAY_SIZE(commands); i++)
if (!strcmp(argv[1], commands[i].cmd))
return commands[i].fn(argc - 1, argv + 1, prefix);
- die(_("fatal: '%s' is not a valid submodule--helper "
+ die(_("'%s' is not a valid submodule--helper "
"subcommand"), argv[1]);
}
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 04/12] submodule update: direct error message to stderr
Reroute the error message for specified but initialized submodules to stderr instead of stdout. Reviewed-by: Jonathan Nieder Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- git-submodule.sh | 4 ++-- t/t7400-submodule-basic.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 9bc5c5f..9ee86d4 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -693,7 +693,7 @@ cmd_update() if test "$update_module" = "none" then - echo "Skipping submodule '$displaypath'" + echo >&2 "Skipping submodule '$displaypath'" continue fi @@ -702,7 +702,7 @@ cmd_update() # Only mention uninitialized submodules when its # path have been specified test "$#" != "0" && - say "$(eval_gettext "Submodule path '\$displaypath' not initialized + say >&2 "$(eval_gettext "Submodule path '\$displaypath' not initialized Maybe you want to use 'update --init'?")" continue fi diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 540771c..5991e3c 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -462,7 +462,7 @@ test_expect_success 'update --init' ' git config --remove-section submodule.example && test_must_fail git config submodule.example.url && - git submodule update init > update.out && + git submodule update init 2> update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir init/.git && @@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' ' mkdir -p sub && ( cd sub && - git submodule update ../init >update.out && + git submodule update ../init 2>update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir ../init/.git && -- 2.7.0.rc0.37.gb7b9e8e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 09/12] git submodule update: have a dedicated helper for cloning
This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.
Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
builtin/submodule--helper.c | 249
git-submodule.sh| 47 +++--
2 files changed, 262 insertions(+), 34 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..e1b60ff 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,254 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
return 0;
}
+struct submodule_update_clone {
+ /* index into 'list', the list of submodules to look into for cloning */
+ int current;
+ struct module_list list;
+ unsigned warn_if_uninitialized : 1;
+
+ /* update parameter passed via commandline */
+ struct submodule_update_strategy update;
+
+ /* configuration parameters which are passed on to the children */
+ int quiet;
+ const char *reference;
+ const char *depth;
+ const char *recursive_prefix;
+ const char *prefix;
+
+ /* Machine-readable status lines to be consumed by git-submodule.sh */
+ struct string_list projectlines;
+
+ /* If we want to stop as fast as possible and return an error */
+ unsigned quickstop : 1;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
+ SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+ STRING_LIST_INIT_DUP, 0}
+
+/**
+ * Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to
+ * run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise.
+ */
+static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
+ struct child_process *child,
+ struct submodule_update_clone *suc,
+ struct strbuf *out)
+{
+ const struct submodule *sub = NULL;
+ struct strbuf displaypath_sb = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *displaypath = NULL;
+ char *url = NULL;
+ int needs_cloning = 0;
+
+ if (ce_stage(ce)) {
+ if (suc->recursive_prefix) {
+ strbuf_addf(out,_("Skipping unmerged submodule
%s/%s\n"),
+ suc->recursive_prefix, ce->name);
+ } else {
+ strbuf_addf(out, _("Skipping unmerged submodule %s\n"),
+ ce->name);
+ }
+ goto cleanup;
+ }
+
+ sub = submodule_from_path(null_sha1, ce->name);
+
+ if (suc->recursive_prefix)
+ displaypath = relative_path(suc->recursive_prefix,
+ ce->name, &displaypath_sb);
+ else
+ displaypath = ce->name;
+
+ if (suc->update.type == SM_UPDATE_NONE
+ || (suc->update.type == SM_UPDATE_UNSPECIFIED
+ && sub->update_strategy.type == SM_UPDATE_NONE)) {
+ strbuf_addf(out, _("Skipping submodule '%s'\n"),
+ displaypath);
+ goto cleanup;
+ }
+
+ /*
+* Looking up the url in .git/config.
+* We must not fall back to .gitmodules as we only want
+* to process configured submodules.
+*/
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "submodule.%s.url", sub->name);
+ git_config_get_string(sb.buf, &url);
+ if (!url) {
+ /*
+* Only mention uninitialized submodules when their
+* path have been specified
+*/
+ if (suc->warn_if_uninitialized)
+ strbuf_addf(out, _("Submodule path '%s' not
initialized\n"
+ "Maybe you want to use 'update --init'?\n"),
+ displaypath);
+ goto cleanup;
+ }
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/.git", ce->name);
+ needs_cloning = !file_exists(sb.buf);
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
+ sha1_to_hex(ce->sha1), ce_stage(ce),
+ needs_cloning, ce->name);
+ string_list_append(&suc->projectlines, sb.buf);
+
+ if (!needs_cloning)
+ goto cleanup;
+
+ child->git_cmd = 1;
+ child->no_stdin = 1;
+ child->stdout_to_stderr = 1;
+ child->err = -1;
+ argv_array_push(&child->args, "submodule--helper");
+
[PATCHv20 05/12] run_processes_parallel: treat output of children as byte array
We do not want the output to be interrupted by a NUL byte, so we
cannot use raw fputs. Introduce strbuf_write to avoid having long
arguments in run-command.c.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
run-command.c | 8
strbuf.c | 6 ++
strbuf.h | 6 ++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/run-command.c b/run-command.c
index 51fd72c..2f8f222 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1011,7 +1011,7 @@ static void pp_cleanup(struct parallel_processes *pp)
* When get_next_task added messages to the buffer in its last
* iteration, the buffered output is non empty.
*/
- fputs(pp->buffered_output.buf, stderr);
+ strbuf_write(&pp->buffered_output, stderr);
strbuf_release(&pp->buffered_output);
sigchain_pop_common();
@@ -1097,7 +1097,7 @@ static void pp_output(struct parallel_processes *pp)
int i = pp->output_owner;
if (pp->children[i].state == GIT_CP_WORKING &&
pp->children[i].err.len) {
- fputs(pp->children[i].err.buf, stderr);
+ strbuf_write(&pp->children[i].err, stderr);
strbuf_reset(&pp->children[i].err);
}
}
@@ -1135,11 +1135,11 @@ static int pp_collect_finished(struct
parallel_processes *pp)
strbuf_addbuf(&pp->buffered_output,
&pp->children[i].err);
strbuf_reset(&pp->children[i].err);
} else {
- fputs(pp->children[i].err.buf, stderr);
+ strbuf_write(&pp->children[i].err, stderr);
strbuf_reset(&pp->children[i].err);
/* Output all other finished child processes */
- fputs(pp->buffered_output.buf, stderr);
+ strbuf_write(&pp->buffered_output, stderr);
strbuf_reset(&pp->buffered_output);
/*
diff --git a/strbuf.c b/strbuf.c
index 38686ff..5f6da82 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -395,6 +395,12 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t
hint)
return cnt;
}
+ssize_t strbuf_write(struct strbuf *sb, FILE *f)
+{
+ return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
+}
+
+
#define STRBUF_MAXLINK (2*PATH_MAX)
int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
diff --git a/strbuf.h b/strbuf.h
index 2bf90e7..d4f2aa1 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -387,6 +387,12 @@ extern ssize_t strbuf_read_file(struct strbuf *sb, const
char *path, size_t hint
extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
/**
+ * Write the whole content of the strbuf to the stream not stopping at
+ * NUL bytes.
+ */
+extern ssize_t strbuf_write(struct strbuf *sb, FILE *stream);
+
+/**
* Read a line from a FILE *, overwriting the existing contents
* of the strbuf. The second argument specifies the line
* terminator character, typically `'\n'`.
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 06/12] run-command: expose default_{start_failure, task_finished}
We want to reuse the error reporting facilities in a later patch.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
run-command.c | 18 +-
run-command.h | 19 +++
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/run-command.c b/run-command.c
index 2f8f222..c9b13cf 100644
--- a/run-command.c
+++ b/run-command.c
@@ -902,10 +902,10 @@ struct parallel_processes {
struct strbuf buffered_output; /* of finished children */
};
-static int default_start_failure(struct child_process *cp,
-struct strbuf *err,
-void *pp_cb,
-void *pp_task_cb)
+int default_start_failure(struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb,
+ void *pp_task_cb)
{
int i;
@@ -916,11 +916,11 @@ static int default_start_failure(struct child_process *cp,
return 0;
}
-static int default_task_finished(int result,
-struct child_process *cp,
-struct strbuf *err,
-void *pp_cb,
-void *pp_task_cb)
+int default_task_finished(int result,
+ struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb,
+ void *pp_task_cb)
{
int i;
diff --git a/run-command.h b/run-command.h
index d5a57f9..a054fa6 100644
--- a/run-command.h
+++ b/run-command.h
@@ -164,6 +164,15 @@ typedef int (*start_failure_fn)(struct child_process *cp,
void *pp_task_cb);
/**
+ * If a command fails to start, then print an error message stating the
+ * exact command which failed.
+ */
+int default_start_failure(struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb,
+ void *pp_task_cb);
+
+/**
* This callback is called on every child process that finished processing.
*
* You must not write to stdout or stderr in this function. Add your
@@ -184,6 +193,16 @@ typedef int (*task_finished_fn)(int result,
void *pp_task_cb);
/**
+ * If the child process returns with a non zero error code, print
+ * an error message of the exact command which failed.
+ */
+int default_task_finished(int result,
+ struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb,
+ void *pp_task_cb);
+
+/**
* Runs up to n processes at the same time. Whenever a process can be
* started, the callback get_next_task_fn is called to obtain the data
* required to start another child process.
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 02/12] submodule-config: drop check against NULL
Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index a5cd2ee..9fa2269 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -267,7 +267,7 @@ static int parse_config(const char *var, const char *value,
void *data)
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->path != NULL)
+ else if (!me->overwrite && submodule->path)
warn_multiple_config(me->commit_sha1, submodule->name,
"path");
else {
@@ -291,7 +291,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->ignore != NULL)
+ else if (!me->overwrite && submodule->ignore)
warn_multiple_config(me->commit_sha1, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
@@ -307,7 +307,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "url")) {
if (!value) {
ret = config_error_nonbool(var);
- } else if (!me->overwrite && submodule->url != NULL) {
+ } else if (!me->overwrite && submodule->url) {
warn_multiple_config(me->commit_sha1, submodule->name,
"url");
} else {
--
2.7.0.rc0.37.gb7b9e8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 08/12] run_processes_parallel: correctly terminate callbacks with an LF
As the strbufs passed around collect all output to the user, and there is no post processing involved we need to care about the line ending ourselves. Reviewed-by: Jonathan Nieder Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- run-command.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/run-command.c b/run-command.c index d2964e1..6fad42f 100644 --- a/run-command.c +++ b/run-command.c @@ -912,6 +912,7 @@ int default_start_failure(struct child_process *cp, strbuf_addstr(out, "Starting a child failed:"); for (i = 0; cp->argv[i]; i++) strbuf_addf(out, " %s", cp->argv[i]); + strbuf_addch(out, '\n'); return 0; } @@ -930,6 +931,7 @@ int default_task_finished(int result, strbuf_addf(out, "A child failed with return code %d:", result); for (i = 0; cp->argv[i]; i++) strbuf_addf(out, " %s", cp->argv[i]); + strbuf_addch(out, '\n'); return 0; } -- 2.7.0.rc0.37.gb7b9e8e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv20 01/12] submodule-config: keep update strategy around
Currently submodule..update is only handled by git-submodule.sh.
C code will start to need to make use of that value as more of the
functionality of git-submodule.sh moves into library code in C.
Add the update field to 'struct submodule' and populate it so it can
be read as sm->update or from sm->update_command.
Reviewed-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 13 +
submodule-config.h | 2 ++
submodule.c| 21 +
submodule.h| 16
4 files changed, 52 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..a5cd2ee 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)
{
free((void *) entry->config->path);
free((void *) entry->config->name);
+ free((void *) entry->config->update_strategy.command);
free(entry->config);
}
@@ -194,6 +195,8 @@ static struct submodule *lookup_or_create_by_name(struct
submodule_cache *cache,
submodule->path = NULL;
submodule->url = NULL;
+ submodule->update_strategy.type = SM_UPDATE_UNSPECIFIED;
+ submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
@@ -311,6 +314,16 @@ static int parse_config(const char *var, const char
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+ } else if (!strcmp(item.buf, "update")) {
+ if (!value)
+ ret = config_error_nonbool(var);
+ else if (!me->overwrite &&
+submodule->update_strategy.type !=
SM_UPDATE_UNSPECIFIED)
+ warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+ else if (parse_submodule_update_strategy(value,
+&submodule->update_strategy) < 0)
+ die(_("invalid value for %s"), var);
}
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..092ebfc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
#define SUBMODULE_CONFIG_CACHE_H
#include "hashmap.h"
+#include "submodule.h"
#include "strbuf.h"
/*
@@ -14,6 +15,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+ struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
};
diff --git a/submodule.c b/submodule.c
index b83939c..b38dd51 100644
--- a/submodule.c
+++ b/submodule.c
@@ -210,6 +210,27 @@ void gitmodules_config(void)
}
}
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst)
+{
+ free((void*)dst->command);
+ dst->command = NULL;
+ if (!strcmp(value, "none"))
+ dst->type = SM_UPDATE_NONE;
+ else if (!strcmp(value, "checkout"))
+ dst->type = SM_UPDATE_CHECKOUT;
+ else if (!strcmp(value, "rebase"))
+ dst->type = SM_UPDATE_REBASE;
+ else if (!strcmp(value, "merge"))
+ dst->type = SM_UPDATE_MERGE;
+ else if (skip_prefix(value, "!", &value)) {
+ dst->type = SM_UPDATE_COMMAND;
+ dst->command = xstrdup(value);
+ } else
+ return -1;
+ return 0;
+}
+
void handle_ignore_submodules_arg(struct diff_options *diffopt,
const char *arg)
{
diff --git a/submodule.h b/submodule.h
index cbc0003..3464500 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,20 @@ enum {
RECURSE_SUBMODULES_ON = 2
};
+enum submodule_update_type {
+ SM_UPDATE_UNSPECIFIED = 0,
+ SM_UPDATE_CHECKOUT,
+ SM_UPDATE_REBASE,
+ SM_UPDATE_MERGE,
+ SM_UPDATE_NONE,
+ SM_UPDATE_COMMAND
+};
+
+struct submodule_update_strategy {
+ enum submodule_update_type type;
+ const char *command;
+};
+
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
int remove_path_from_gitmodules(const char *path);
@@ -21,6 +35,8 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst);
void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
--
2.7.0.rc0.37.gb7b9e8
Re: [PATCH] http: honor no_http env variable to bypass proxy
Jiang Xin writes:
> From: Jiang Xin
>
> Curl and its families honor several proxy related environment variables:
>
> * http_proxy and https_proxy define proxy for http/https connections.
> * no_proxy (a comma separated hosts) defines hosts bypass the proxy.
>
> This command will bypass the bad-proxy and connect to the host directly:
>
> no_proxy=* https_proxy=http://bad-proxy/ \
> curl -sk https://google.com/
>
> Before commit 372370f (http: use credential API to handle proxy auth...),
> Environment variable "no_proxy" will take effect if the config variable
> "http.proxy" is not set. So the following comamnd won't fail if not
> behind a firewall.
>
> no_proxy=* https_proxy=http://bad-proxy/ \
> git ls-remote https://github.com/git/git
>
> But commit 372370f not only read git config variable "http.proxy", but
> also read "http_proxy" and "https_proxy" environment variables, and set
> the curl option using:
>
> curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
>
> This caused "no_proxy" environment variable not working any more.
>
> Set extra curl option "CURLOPT_NOPROXY" will fix this issue.
>
> Signed-off-by: Jiang Xin
> ---
> http.c | 6 ++
> 1 file changed, 6 insertions(+)
Sounds sensible; I am guessing that this is 2.8.0-rc0 regression
that we need to fast-track?
Knut, does this look good?
Thanks.
> diff --git a/http.c b/http.c
> index 1d5e3bb..69da445 100644
> --- a/http.c
> +++ b/http.c
> @@ -70,6 +70,7 @@ static long curl_low_speed_limit = -1;
> static long curl_low_speed_time = -1;
> static int curl_ftp_no_epsv;
> static const char *curl_http_proxy;
> +static const char *curl_no_proxy;
> static const char *http_proxy_authmethod;
> static struct {
> const char *name;
> @@ -624,6 +625,11 @@ static CURL *get_curl_handle(void)
> }
>
> curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
> +#if LIBCURL_VERSION_NUM >= 0x071304
> + var_override(&curl_no_proxy, getenv("NO_PROXY"));
> + var_override(&curl_no_proxy, getenv("no_proxy"));
> + curl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);
> +#endif
> }
> init_curl_proxy_auth(result);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv20 00/12] Expose submodule parallelism to the user
Stefan Beller wrote:
> I added your suggestions as amending and as a new patch.
I think we're at the point where patches on top would work better. I
admit I was a little scared to see another reroll.
[...]
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -299,10 +299,10 @@ static int prepare_to_clone_next_submodule(const struct
> cache_entry *ce,
>
> if (ce_stage(ce)) {
> if (suc->recursive_prefix) {
> - strbuf_addf(out, "Skipping unmerged submodule %s/%s\n",
> + strbuf_addf(out,_("Skipping unmerged submodule
> %s/%s\n"),
Missing space after comma.
Usual practice for i18n would be something like
struct strbuf path = STRBUF_INIT;
if (suc->recursive_prefix)
strbuf_addf(&path, "%s/%s", suc->recursive_prefix, ce->name);
else
strbuf_addstr(&path, ce->name);
strbuf_addf(out, _("Skipping unmerged submodule %s"), path.buf);
strbuf_addch(out, '\n');
strbuf_release(&path);
Reasons:
- translators shouldn't have to worry about the trailing newline
- minimizing number of strings to translate
- minimizing the chance that a translator typo produces an invalid path
[...]
> @@ -319,7 +319,7 @@ static int prepare_to_clone_next_submodule(const struct
> cache_entry *ce,
> if (suc->update.type == SM_UPDATE_NONE
> || (suc->update.type == SM_UPDATE_UNSPECIFIED
> && sub->update_strategy.type == SM_UPDATE_NONE)) {
> - strbuf_addf(out, "Skipping submodule '%s'\n",
> + strbuf_addf(out, _("Skipping submodule '%s'\n"),
> displaypath);
Same issue here with the trailing \n.
If the strbuf_addf + strbuf_addch('\n') pattern is common enough, we
could introduce a helper (e.g. strbuf_addf_nl) to save typing.
Thanks and hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Change type of signed int flags to unsigned
Saurav Sachidanand writes:
> “pattern” and “exclude” are two structs defined in attr.c and dir.h
> respectively. Each contain a “flags” field of type int that takes on
> values from the set of positive integers {1, 4, 8, 16}, that are
> enumerated through the macro EXC_FLAG_*.
>
> That the most significant bit (used to store the sign) of these two
> fields is not used in any special way is observed from the fact that
> the "flags" fields (accessed within attr.c, dir.c, and
> builtin/check-ignore.c) are either checked for their values using the
> & operator (e.g.: flags & EXC_FLAG_NODIR), or assigned a value of 0
> first and then assigned any one of {1, 4, 8, 16} using the | operator
> (e.g.: flags |= EXC_FLAG_NODIR). Hence, change the type of "flags"
> to unsigned in both structs.
>
> Furthermore, “flags” of both structs is passed by value or by reference
> to the functions parse_exclude_pattern, match_basename and
> match_pathname (declared in dir.h), that have an “int” or “int *” type
> for "flags" in their signatures. To avoid implicit conversion to types,
> or pointers to types, of different sign, change the type for “flags” to
> “unsigned” and “unsigned *” in the respective function signatures.
>
> And while we’re at it, document the "flags" field of "exclude" to
> explicitly state the values it’s supposed to take on.
The above is overly verbose for two reasons, I think.
* You seem to start from "I have to change the type of struct
fields", which leads you to describe "these fields are passed as
parameters to functions, so their signatures also need to change"
cascade of changes. Instead, think of this change is about
"store EXC_FLAG_* flags in an unsigned integer" (which can be the
title of the patch). Then the first and the third paragraphs do
not have to even exist ;-)
* The second paragraph could just be:
No variable (or structure field) that hold these bits is tested
for its MSB with (variable < 0), so there is no reason to hold
them in a signed integer.
Here, also notice that your version stressed "of these two
fields" too much; the reasoning applies equally to any variable
that holds copies of values from these fields (or in general,
anything that holds combination of EXC_FLAG_* flags).
Otherwise it looks good to me.
Thanks.
> Signed-off-by: Saurav Sachidanand
> ---
>
> This patch is for the suggested microproject for GSoC 2016 titled
> "Use unsigned integral type for collection of bits." It is the third
> patch for this project (technically second, considering only the
> subject of the email) that incorporates changes to the commit message
> suggested by Moritz Neeb and Eric Sunshine, and to some function
> signatures suggested by Duy Nguyen. Thanks to them for their feedback.
>
> attr.c | 2 +-
> dir.c | 8
> dir.h | 8
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 086c08d..679e13c 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -124,7 +124,7 @@ struct pattern {
> const char *pattern;
> int patternlen;
> int nowildcardlen;
> - int flags; /* EXC_FLAG_* */
> + unsigned flags; /* EXC_FLAG_* */
> };
>
> /*
> diff --git a/dir.c b/dir.c
> index 552af23..82cec7d 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -459,7 +459,7 @@ int no_wildcard(const char *string)
>
> void parse_exclude_pattern(const char **pattern,
> int *patternlen,
> -int *flags,
> +unsigned *flags,
> int *nowildcardlen)
> {
> const char *p = *pattern;
> @@ -500,7 +500,7 @@ void add_exclude(const char *string, const char *base,
> {
> struct exclude *x;
> int patternlen;
> - int flags;
> + unsigned flags;
> int nowildcardlen;
>
> parse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);
> @@ -811,7 +811,7 @@ void add_excludes_from_file(struct dir_struct *dir, const
> char *fname)
>
> int match_basename(const char *basename, int basenamelen,
> const char *pattern, int prefix, int patternlen,
> -int flags)
> +unsigned flags)
> {
> if (prefix == patternlen) {
> if (patternlen == basenamelen &&
> @@ -836,7 +836,7 @@ int match_basename(const char *basename, int basenamelen,
> int match_pathname(const char *pathname, int pathlen,
> const char *base, int baselen,
> const char *pattern, int prefix, int patternlen,
> -int flags)
> +unsigned flags)
> {
> const char *name;
> int namelen;
> diff --git a/dir.h b/dir.h
> index 3ec3fb0..e942b50 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -28,7 +28,7 @@ struct exclude {
> int nowildcardlen;
> const char *base;
> int baselen;
> - int flags;
> + unsigned flags; /* EXC_FLAG_* */
>
> /*
>* Counting start
Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()
On 02/29/2016 07:19 PM, Eric Sunshine wrote:
> If you do elect to keep things the way they are, then (as mentioned in
> my v2 review) it would be helpful for the above paragraph to explain
> that strbuf_split() leave the "terminator" on the split elements, thus
> clarifying why the rtrim() of split[0] is still needed.
>
Yes I would rather leave it like it is. I have the feeling it is
unmotivated to remove the rtrim of split[1] in the patch 5/7, because it
is directly related to the strbuf_getline_lf() replacement. Thats's what
I was trying to explain in the 2nd paragraph of the commit message.
First I was following your review, but then I had to add a paragraph in
patch 5/7 that says something like "because the effect of the previous
patch is that there is not a CR anymore, we can now safely remove
rtrim() split[1]."
You're right, maybe I should add a comment about why I left rtrim() of
split[0] to make it more obvious. I thought that would get clear by
looking at the context, i.e. patch 5/7, where it is explained (by you,
thanks for that), that strbuf_split leave this space. Is the assumption,
that those two patches are most times viewed in context wrong?
Thanks,
Moritz
>> Signed-off-by: Moritz Neeb
>> ---
>> builtin/notes.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index ed6f222..706ec11 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char
>> *rewrite_cmd)
>> t = &default_notes_tree;
>> }
>>
>> - while (strbuf_getline_lf(&buf, stdin) != EOF) {
>> + while (strbuf_getline(&buf, stdin) != EOF) {
>> unsigned char from_obj[20], to_obj[20];
>> struct strbuf **split;
>> int err;
>> @@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char
>> *rewrite_cmd)
>> if (!split[0] || !split[1])
>> die(_("Malformed input line: '%s'."), buf.buf);
>> strbuf_rtrim(split[0]);
>> - strbuf_rtrim(split[1]);
>> if (get_sha1(split[0]->buf, from_obj))
>> die(_("Failed to resolve '%s' as a valid ref."),
>> split[0]->buf);
>> if (get_sha1(split[1]->buf, to_obj))
>> --
>> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count
On Mon, Feb 29, 2016 at 9:49 AM, Junio C Hamano wrote: > Jacob Keller writes: >> Fix the usage description to match implementation. Add an argc check to >> enforce no extra arguments. > > The above sounds very sensible. > Right. >> Fix a bug in the argument passing in >> git-submodule.sh which would pass --reference and --depth as empty >> strings when they were unused, resulting in extra argc after parsing >> options. > > This does make sense but it is an unrelated fix. Perhaps split this > patch into two? > That actually is required because otherwise adding a check for argc would break the things. I could split them and do this first and then check for argc if you really prefer? >> + if (argc) >> + usage(*git_submodule_helper_usage); >> + > > That asterisk looks very unusual and wanting to be future-proofed > (i.e. who says that only the first entry matters?). Should't this > be calling usage_with_options()? > I... didn't know usage_with_options was a thing! Hah. I can fix these up. Thanks, Jake\ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line
On Mon, Feb 29, 2016 at 10:20 AM, Junio C Hamano wrote:
> Jacob Keller writes:
>
>> +static int sanitize_submodule_config(const char *var, const char *value,
>> void *data)
>> +{
>> + struct strbuf quoted = STRBUF_INIT;
>> + struct strbuf *out = data;
>> +
>> + if (submodule_config_ok(var)) {
>> + if (out->len)
>> + strbuf_addch(out, ' ');
>> +
>> + sq_quotef(out, "%s=%s", var, value);
>
> Can a configuration variable that comes from the original command
> line be a boolean true that is spelled without "=true", i.e. can
> value be NULL here?
>
Wouldn't it just be the empty string? I'm not sure. I suppose I can do:
sq_quotef(out, "%s=%s, var, value ? value : "")
Or something?
Thanks
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count
Jacob Keller writes: >> This does make sense but it is an unrelated fix. Perhaps split this >> patch into two? > > That actually is required because otherwise adding a check for argc > would break the things. I could split them and do this first and then > check for argc if you really prefer? It is not "check for argc breaks", it is already broken and by checking for argc you are exposing the breakage, no? So I'd say fix that first and then fix the clone subcommand? > >>> + if (argc) >>> + usage(*git_submodule_helper_usage); >>> + >> >> That asterisk looks very unusual and wanting to be future-proofed >> (i.e. who says that only the first entry matters?). Should't this >> be calling usage_with_options()? >> > > I... didn't know usage_with_options was a thing! Hah. I can fix these up. > > Thanks, > Jake\ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line
Jacob Keller writes:
> On Mon, Feb 29, 2016 at 10:20 AM, Junio C Hamano wrote:
>> Jacob Keller writes:
>>
>>> +static int sanitize_submodule_config(const char *var, const char *value,
>>> void *data)
>>> +{
>>> + struct strbuf quoted = STRBUF_INIT;
>>> + struct strbuf *out = data;
>>> +
>>> + if (submodule_config_ok(var)) {
>>> + if (out->len)
>>> + strbuf_addch(out, ' ');
>>> +
>>> + sq_quotef(out, "%s=%s", var, value);
>>
>> Can a configuration variable that comes from the original command
>> line be a boolean true that is spelled without "=true", i.e. can
>> value be NULL here?
>>
>
> Wouldn't it just be the empty string?
I was talking about the "not even an equal sign" true, i.e.
$ git -c random.what -c random.false=no -c random.true=yes \
config --bool --get-regexp 'random.*'
random.what true
random.false false
random.true true
What would you see for random.what in the above function (if the
callchain allowed you to pass it through)?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()
On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb wrote: > On 02/29/2016 07:19 PM, Eric Sunshine wrote: >> If you do elect to keep things the way they are, then (as mentioned in >> my v2 review) it would be helpful for the above paragraph to explain >> that strbuf_split() leave the "terminator" on the split elements, thus >> clarifying why the rtrim() of split[0] is still needed. > > Yes I would rather leave it like it is. I have the feeling it is > unmotivated to remove the rtrim of split[1] in the patch 5/7, because it > is directly related to the strbuf_getline_lf() replacement. Thats's what > I was trying to explain in the 2nd paragraph of the commit message. > > First I was following your review, but then I had to add a paragraph in > patch 5/7 that says something like "because the effect of the previous > patch is that there is not a CR anymore, we can now safely remove > rtrim() split[1]." > > You're right, maybe I should add a comment about why I left rtrim() of > split[0] to make it more obvious. I thought that would get clear by > looking at the context, i.e. patch 5/7, where it is explained (by you, > thanks for that), that strbuf_split leave this space. Is the assumption, > that those two patches are most times viewed in context wrong? I was more concerned about someone reading patch 4/7 in isolation and not consulting 5/7 (which might happen during a "blame" session, but it's a very minor point, not worth a re-roll if you and Junio are happy with the series as is. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv20 00/12] Expose submodule parallelism to the user
On Mon, Feb 29, 2016 at 11:32 AM, Jonathan Nieder wrote:
> Stefan Beller wrote:
>
>> I added your suggestions as amending and as a new patch.
>
> I think we're at the point where patches on top would work better. I
> admit I was a little scared to see another reroll.
Yeah I am a bit scared too, so I'll do patches on top for further fixes
after one last reroll, fixing the issues below.
>
> [...]
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -299,10 +299,10 @@ static int prepare_to_clone_next_submodule(const
>> struct cache_entry *ce,
>>
>> if (ce_stage(ce)) {
>> if (suc->recursive_prefix) {
>> - strbuf_addf(out, "Skipping unmerged submodule %s/%s\n",
>> + strbuf_addf(out,_("Skipping unmerged submodule
>> %s/%s\n"),
>
> Missing space after comma.
>
> Usual practice for i18n would be something like
>
> struct strbuf path = STRBUF_INIT;
> if (suc->recursive_prefix)
> strbuf_addf(&path, "%s/%s", suc->recursive_prefix, ce->name);
> else
> strbuf_addstr(&path, ce->name);
>
> strbuf_addf(out, _("Skipping unmerged submodule %s"), path.buf);
> strbuf_addch(out, '\n');
> strbuf_release(&path);
>
> Reasons:
> - translators shouldn't have to worry about the trailing newline
> - minimizing number of strings to translate
> - minimizing the chance that a translator typo produces an invalid path
Thanks for reminding me of that practice!
>
> [...]
>> @@ -319,7 +319,7 @@ static int prepare_to_clone_next_submodule(const struct
>> cache_entry *ce,
>> if (suc->update.type == SM_UPDATE_NONE
>> || (suc->update.type == SM_UPDATE_UNSPECIFIED
>> && sub->update_strategy.type == SM_UPDATE_NONE)) {
>> - strbuf_addf(out, "Skipping submodule '%s'\n",
>> + strbuf_addf(out, _("Skipping submodule '%s'\n"),
>> displaypath);
>
> Same issue here with the trailing \n.
>
> If the strbuf_addf + strbuf_addch('\n') pattern is common enough, we
> could introduce a helper (e.g. strbuf_addf_nl) to save typing.
I don't think it is common enough yet.
Thanks,
Stefan
>
> Thanks and hope that helps,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/32] refs: add a backend method structure with transaction functions
On Fri, 2016-02-26 at 23:06 -0500, Jeff King wrote:
> On Wed, Feb 24, 2016 at 05:58:37PM -0500, David Turner wrote:
>
> > +int set_ref_storage_backend(const char *name)
> > +{
> > + struct ref_storage_be *be;
> > +
> > + for (be = refs_backends; be; be = be->next)
> > + if (!strcmp(be->name, name)) {
> > + the_refs_backend = be;
> > + return 0;
> > + }
> > + return 1;
> > +}
>
> So we search through the list and assign the_refs_backend if we find
> something, returning 0 for success, and 1 if we found nothing. OK
> (though our usual convention is that if 0 is success, -1 is the error
> case).
Will fix.
> > +int ref_storage_backend_exists(const char *name)
> > +{
> > + struct ref_storage_be *be;
> > +
> > + for (be = refs_backends; be; be = be->next)
> > + if (!strcmp(be->name, name)) {
> > + the_refs_backend = be;
> > + return 1;
> > + }
> > + return 0;
> > +}
>
> Here we do the same thing, but we get "1" for exists, and "0"
> otherwise. That makes sense if this is about querying for existence.
> But
> why does the query function still set the_refs_backend?
Yeah, that's wrong.
> I'm guessing the assignment in the second one is just copy-pasta, but
> maybe the whole thing would be simpler if they could share the
> implementation, like:
>
> struct ref_storage_be *find_ref_storage_backend(const char *name)
> {
> struct ref_storage *be;
> for (be = refs_backends; be; be = be->next)
> if (!strcmp(be->name, name))
> return be;
> return NULL;
> }
>
> int set_ref_storage_backend(const char *name)
> {
> struct ref_storage *be = find_ref_storage_backend(name);
> if (!be)
> return -1;
> the_refs_backend = be;
> return 0;
> }
>
> You don't really need an "exists", as you can check that "find"
> doesn't
> return NULL, but you could wrap it, of course.
I'd rather wrap it to keep the backends firmly inside the refs-internal
barrier.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv19 00/11] Expose submodule parallelism to the user
Hi folks,
we have a major breakage in the parallel tasks infrastructure, and I'm
afraid it is already in master.
Instrument the code in sb/submodule-parallel-update like this and enjoy
the fireworks of './t7400-submodule-basic.sh -v -i -x --debug':
diff --git a/git-submodule.sh b/git-submodule.sh
index 0322282..482c7f6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -690,8 +690,9 @@ cmd_update()
cmd_init "--" "$@" || return
fi
+ set -x
{
- git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
+ valgrind git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5572327..717e491 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -337,6 +337,7 @@ test_expect_success 'update should fail when path is used
by a file' '
echo "hello" >init &&
test_must_fail git submodule update &&
+ false &&
test_cmp expect init
'
The culprit seems to be default_task_finished(), which accesses argv[]
of the struct child_process after finish_command has released it,
provided the child exited with an error, for example:
==3395== Invalid read of size 8
==3395==at 0x54F991: default_task_finished (run-command.c:932)
==3395==by 0x49158F: update_clone_task_finished (submodule--helper.c:421)
==3395==by 0x5504A2: pp_collect_finished (run-command.c:1122)
==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
==3395==by 0x405CBE: run_builtin (git.c:353)
==3395==by 0x405EAA: handle_builtin (git.c:540)
==3395==by 0x405FCC: run_argv (git.c:594)
==3395==by 0x4061BF: main (git.c:701)
==3395== Address 0x5e49370 is 0 bytes inside a block of size 192 free'd
==3395==at 0x4C2A37C: free (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3395==by 0x4A26EE: argv_array_clear (argv-array.c:73)
==3395==by 0x54DFC4: child_process_clear (run-command.c:18)
==3395==by 0x54EFA7: finish_command (run-command.c:539)
==3395==by 0x550413: pp_collect_finished (run-command.c:1120)
==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
==3395==by 0x405CBE: run_builtin (git.c:353)
==3395==by 0x405EAA: handle_builtin (git.c:540)
==3395==by 0x405FCC: run_argv (git.c:594)
==3395==by 0x4061BF: main (git.c:701)
I haven't thought about a solution, yet. Perhaps you have ideas.
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv20 00/12] Expose submodule parallelism to the user
Jonathan Nieder writes: > Stefan Beller wrote: > >> I added your suggestions as amending and as a new patch. > > I think we're at the point where patches on top would work better. I > admit I was a little scared to see another reroll. Yup, let's stop rerolling this during pre-release period, and instead concentrate on fixing what's already in 'master' ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv19 00/11] Expose submodule parallelism to the user
On Mon, Feb 29, 2016 at 12:48 PM, Johannes Sixt wrote:
> Hi folks,
>
> we have a major breakage in the parallel tasks infrastructure, and I'm
> afraid it is already in master.
>
> Instrument the code in sb/submodule-parallel-update like this and enjoy
> the fireworks of './t7400-submodule-basic.sh -v -i -x --debug':
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 0322282..482c7f6 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -690,8 +690,9 @@ cmd_update()
> cmd_init "--" "$@" || return
> fi
>
> + set -x
> {
> - git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
> + valgrind git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
> ${wt_prefix:+--prefix "$wt_prefix"} \
> ${prefix:+--recursive-prefix "$prefix"} \
> ${update:+--update "$update"} \
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 5572327..717e491 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -337,6 +337,7 @@ test_expect_success 'update should fail when path is used
> by a file' '
>
> echo "hello" >init &&
> test_must_fail git submodule update &&
> + false &&
>
> test_cmp expect init
> '
>
> The culprit seems to be default_task_finished(), which accesses argv[]
> of the struct child_process after finish_command has released it,
> provided the child exited with an error, for example:
>
> ==3395== Invalid read of size 8
> ==3395==at 0x54F991: default_task_finished (run-command.c:932)
> ==3395==by 0x49158F: update_clone_task_finished (submodule--helper.c:421)
> ==3395==by 0x5504A2: pp_collect_finished (run-command.c:1122)
> ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
> ==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
> ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
> ==3395==by 0x405CBE: run_builtin (git.c:353)
> ==3395==by 0x405EAA: handle_builtin (git.c:540)
> ==3395==by 0x405FCC: run_argv (git.c:594)
> ==3395==by 0x4061BF: main (git.c:701)
> ==3395== Address 0x5e49370 is 0 bytes inside a block of size 192 free'd
> ==3395==at 0x4C2A37C: free (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3395==by 0x4A26EE: argv_array_clear (argv-array.c:73)
> ==3395==by 0x54DFC4: child_process_clear (run-command.c:18)
> ==3395==by 0x54EFA7: finish_command (run-command.c:539)
> ==3395==by 0x550413: pp_collect_finished (run-command.c:1120)
> ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
> ==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
> ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
> ==3395==by 0x405CBE: run_builtin (git.c:353)
> ==3395==by 0x405EAA: handle_builtin (git.c:540)
> ==3395==by 0x405FCC: run_argv (git.c:594)
> ==3395==by 0x4061BF: main (git.c:701)
>
> I haven't thought about a solution, yet. Perhaps you have ideas.
>
> -- Hannes
>
What about unfolding finish_command like so:
diff --git a/run-command.c b/run-command.c
index 863dad5..659abd9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1115,11 +1115,13 @@ static int pp_collect_finished(struct
parallel_processes *pp)
if (i == pp->max_processes)
break;
- code = finish_command(&pp->children[i].process);
+ code = wait_or_whine(pp->children[i].process.pid,
+pp->children[i].process.argv[0], 0);
code = pp->task_finished(code, &pp->children[i].process,
&pp->children[i].err, pp->data,
&pp->children[i].data);
+ child_process_clear(&pp->children[i].process);
if (code)
result = code;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv19 00/11] Expose submodule parallelism to the user
Johannes Sixt writes: > The culprit seems to be default_task_finished(), which accesses argv[] > of the struct child_process after finish_command has released it, > provided the child exited with an error, for example: Thanks for a report. > ==3395== Invalid read of size 8 > ==3395==at 0x54F991: default_task_finished (run-command.c:932) That one and also start_failure() do run after child_process_clear() has cleaned things up, so they shouldn't be looking at argv[] (or anything in that structure for that matter). > ==3395==by 0x49158F: update_clone_task_finished (submodule--helper.c:421) > ==3395==by 0x5504A2: pp_collect_finished (run-command.c:1122) > ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194) > ==3395==by 0x4918EB: update_clone (submodule--helper.c:483) > ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) > ==3395==by 0x405CBE: run_builtin (git.c:353) > ==3395==by 0x405EAA: handle_builtin (git.c:540) > ==3395==by 0x405FCC: run_argv (git.c:594) > ==3395==by 0x4061BF: main (git.c:701) > ==3395== Address 0x5e49370 is 0 bytes inside a block of size 192 free'd > ==3395==at 0x4C2A37C: free (in > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > ==3395==by 0x4A26EE: argv_array_clear (argv-array.c:73) > ==3395==by 0x54DFC4: child_process_clear (run-command.c:18) > ==3395==by 0x54EFA7: finish_command (run-command.c:539) > ==3395==by 0x550413: pp_collect_finished (run-command.c:1120) > ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194) > ==3395==by 0x4918EB: update_clone (submodule--helper.c:483) > ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) > ==3395==by 0x405CBE: run_builtin (git.c:353) > ==3395==by 0x405EAA: handle_builtin (git.c:540) > ==3395==by 0x405FCC: run_argv (git.c:594) > ==3395==by 0x4061BF: main (git.c:701) > > I haven't thought about a solution, yet. Perhaps you have ideas. > > -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Git has been accepted as a GSoC 2016 mentor organization!
Hi, This is my pleasure to inform you all that Git has just been accepted as a GSoC mentor organization. https://summerofcode.withgoogle.com/organizations/?sp-page=3 I've invited Johannes, Stefan, Christian and Lars as potential mentors for Git, and Carlos to represent libgit2. I also took the freedom to invite Junio, not really as potential mentor, but to give access to students proposals and give an opportunity to comment. Let me (or Peff) know if you want an invitation too. No commitment to mentor anyone for now if you accept the invitation, this will be decided later. As a reminder, we post all the GSoC related stuff here: http://git.github.io/SoC-2016-Ideas/ http://git.github.io/SoC-2016-Microprojects/ Cheers! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv19 00/11] Expose submodule parallelism to the user
On Mon, Feb 29, 2016 at 1:01 PM, Junio C Hamano wrote: > Johannes Sixt writes: > >> The culprit seems to be default_task_finished(), which accesses argv[] >> of the struct child_process after finish_command has released it, >> provided the child exited with an error, for example: > > Thanks for a report. > >> ==3395== Invalid read of size 8 >> ==3395==at 0x54F991: default_task_finished (run-command.c:932) > > That one and also start_failure() do run after child_process_clear() > has cleaned things up, so they shouldn't be looking at argv[] (or > anything in that structure for that matter). I am undecided if easy access to the child process struct is a benefit or not for the callback. It makes error reporting easy, but maybe hacky as you poke around with the argv. Maybe we want to remove the struct child_process from the function signature of the callbacks and callbacks need to rely on the data provided solely thru the pointer as passed around for callback purposes, which the user is free to use for any kind of data. As a rather quickfix for 2.8 (and 2.7) we could however just breakup the finish_command function and call child_process_clear after the callbacks have run. > > > >> ==3395==by 0x49158F: update_clone_task_finished (submodule--helper.c:421) >> ==3395==by 0x5504A2: pp_collect_finished (run-command.c:1122) >> ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194) >> ==3395==by 0x4918EB: update_clone (submodule--helper.c:483) >> ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) >> ==3395==by 0x405CBE: run_builtin (git.c:353) >> ==3395==by 0x405EAA: handle_builtin (git.c:540) >> ==3395==by 0x405FCC: run_argv (git.c:594) >> ==3395==by 0x4061BF: main (git.c:701) >> ==3395== Address 0x5e49370 is 0 bytes inside a block of size 192 free'd >> ==3395==at 0x4C2A37C: free (in >> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==3395==by 0x4A26EE: argv_array_clear (argv-array.c:73) >> ==3395==by 0x54DFC4: child_process_clear (run-command.c:18) >> ==3395==by 0x54EFA7: finish_command (run-command.c:539) >> ==3395==by 0x550413: pp_collect_finished (run-command.c:1120) >> ==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194) >> ==3395==by 0x4918EB: update_clone (submodule--helper.c:483) >> ==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) >> ==3395==by 0x405CBE: run_builtin (git.c:353) >> ==3395==by 0x405EAA: handle_builtin (git.c:540) >> ==3395==by 0x405FCC: run_argv (git.c:594) >> ==3395==by 0x4061BF: main (git.c:701) >> >> I haven't thought about a solution, yet. Perhaps you have ideas. >> >> -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
