date:20130730

Hi,

While going through the for-each-ref-pretty series that Duy and I were
developing, I noticed that this cleanup was independent and good
as-it-is.

So here it is.

Nguyễn Thái Ngọc Duy (1):
  for-each-ref, quote: convert *_quote_print - *_quote_buf

Ramkumar Ramachandra (2):
  tar-tree: remove dependency on sq_quote_print()
  quote: remove sq_quote_print()

 builtin/for-each-ref.c | 13 +++
 builtin/tar-tree.c | 11 +
 quote.c| 61 ++
 quote.h|  8 +++
 4 files changed, 39 insertions(+), 54 deletions(-)

-- 
1.8.4.rc0.4.g4634265

--
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 1/3] for-each-ref, quote: convert _quote_print - _quote_buf

From: Nguyễn Thái Ngọc Duy pclo...@gmail.com

for-each-ref.c:print_value() currently prints values to stdout
immediately using {sq|perl|python|tcl}_quote_print, giving us no
opportunity to do any further processing.  In preparation for getting
print_value() to accept an additional strbuf argument to write to,
convert the *_quote_print functions and callers to *_quote_buf.

[rr: commit message, minor modifications]

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 builtin/for-each-ref.c | 13 +
 quote.c| 44 ++--
 quote.h|  6 +++---
 3 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..1d4083c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -867,24 +867,29 @@ static void sort_refs(struct ref_sort *sort, struct 
refinfo **refs, int num_refs
 static void print_value(struct refinfo *ref, int atom, int quote_style)
 {
struct atom_value *v;
+   struct strbuf sb = STRBUF_INIT;
get_value(ref, atom, v);
switch (quote_style) {
case QUOTE_NONE:
fputs(v-s, stdout);
break;
case QUOTE_SHELL:
-   sq_quote_print(stdout, v-s);
+   sq_quote_buf(sb, v-s);
break;
case QUOTE_PERL:
-   perl_quote_print(stdout, v-s);
+   perl_quote_buf(sb, v-s);
break;
case QUOTE_PYTHON:
-   python_quote_print(stdout, v-s);
+   python_quote_buf(sb, v-s);
break;
case QUOTE_TCL:
-   tcl_quote_print(stdout, v-s);
+   tcl_quote_buf(sb, v-s);
break;
}
+   if (quote_style != QUOTE_NONE) {
+   fputs(sb.buf, stdout);
+   strbuf_release(sb);
+   }
 }
 
 static int hex1(char ch)
diff --git a/quote.c b/quote.c
index 5c88081..9fd66c6 100644
--- a/quote.c
+++ b/quote.c
@@ -408,72 +408,72 @@ int unquote_c_style(struct strbuf *sb, const char 
*quoted, const char **endp)
 
 /* quoting as a string literal for other languages */
 
-void perl_quote_print(FILE *stream, const char *src)
+void perl_quote_buf(struct strbuf *sb, const char *src)
 {
const char sq = '\'';
const char bq = '\\';
char c;
 
-   fputc(sq, stream);
+   strbuf_addch(sb, sq);
while ((c = *src++)) {
if (c == sq || c == bq)
-   fputc(bq, stream);
-   fputc(c, stream);
+   strbuf_addch(sb, bq);
+   strbuf_addch(sb, c);
}
-   fputc(sq, stream);
+   strbuf_addch(sb, sq);
 }
 
-void python_quote_print(FILE *stream, const char *src)
+void python_quote_buf(struct strbuf *sb, const char *src)
 {
const char sq = '\'';
const char bq = '\\';
const char nl = '\n';
char c;
 
-   fputc(sq, stream);
+   strbuf_addch(sb, sq);
while ((c = *src++)) {
if (c == nl) {
-   fputc(bq, stream);
-   fputc('n', stream);
+   strbuf_addch(sb, bq);
+   strbuf_addch(sb, 'n');
continue;
}
if (c == sq || c == bq)
-   fputc(bq, stream);
-   fputc(c, stream);
+   strbuf_addch(sb, bq);
+   strbuf_addch(sb, c);
}
-   fputc(sq, stream);
+   strbuf_addch(sb, sq);
 }
 
-void tcl_quote_print(FILE *stream, const char *src)
+void tcl_quote_buf(struct strbuf *sb, const char *src)
 {
char c;
 
-   fputc('', stream);
+   strbuf_addch(sb, '');
while ((c = *src++)) {
switch (c) {
case '[': case ']':
case '{': case '}':
case '$': case '\\': case '':
-   fputc('\\', stream);
+   strbuf_addch(sb, '\\');
default:
-   fputc(c, stream);
+   strbuf_addch(sb, c);
break;
case '\f':
-   fputs(\\f, stream);
+   strbuf_addstr(sb, \\f);
break;
case '\r':
-   fputs(\\r, stream);
+   strbuf_addstr(sb, \\r);
break;
case '\n':
-   fputs(\\n, stream);
+   strbuf_addstr(sb, \\n);
break;
case '\t':
-   fputs(\\t, stream);
+   strbuf_addstr(sb, \\t);
break;
case '\v':
-   fputs(\\v, stream);
+   strbuf_addstr(sb, \\v);
break;

[PATCH 3/3] quote: remove sq_quote_print()

Remove sq_quote_print() since it has no callers.  A nicer alternative
sq_quote_buf() exists: its callers aren't forced to print immediately.

For historical context, sq_quote_print() was first introduced in
575ba9d6 (GIT_TRACE: show which built-in/external commands are executed,
2006-06-25) for the purpose of printing argv for $GIT_TRACE.  Today, we
achieve this using trace_argv_printf() - sq_quote_argv() -
sq_quote_buf(), which ultimately fills in a strbuf.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 quote.c | 17 -
 quote.h |  2 --
 2 files changed, 19 deletions(-)

diff --git a/quote.c b/quote.c
index 9fd66c6..45e3db1 100644
--- a/quote.c
+++ b/quote.c
@@ -42,23 +42,6 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
free(to_free);
 }
 
-void sq_quote_print(FILE *stream, const char *src)
-{
-   char c;
-
-   fputc('\'', stream);
-   while ((c = *src++)) {
-   if (need_bs_quote(c)) {
-   fputs('\\, stream);
-   fputc(c, stream);
-   fputc('\'', stream);
-   } else {
-   fputc(c, stream);
-   }
-   }
-   fputc('\'', stream);
-}
-
 void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
 {
int i;
diff --git a/quote.h b/quote.h
index 6996ebd..71dcc3a 100644
--- a/quote.h
+++ b/quote.h
@@ -27,8 +27,6 @@ struct strbuf;
  * excluding the final null regardless of the buffer size.
  */
 
-extern void sq_quote_print(FILE *stream, const char *src);
-
 extern void sq_quote_buf(struct strbuf *, const char *src);
 extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
 
-- 
1.8.4.rc0.4.g4634265

--
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 2/3] tar-tree: remove dependency on sq_quote_print()

Currently, there is exactly one caller of sq_quote_print(), namely
cmd_tar_tree().  In the interest of removing sq_quote_print() and
simplification, replace it with an equivalent call to sq_quote_argv().
No functional changes intended.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 builtin/tar-tree.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/tar-tree.c b/builtin/tar-tree.c
index 3f1e701..ba3ffe6 100644
--- a/builtin/tar-tree.c
+++ b/builtin/tar-tree.c
@@ -26,8 +26,8 @@ int cmd_tar_tree(int argc, const char **argv, const char 
*prefix)
 * $0 tree-ish basedir ==
 *  git archive --format-tar --prefix=basedir tree-ish
 */
-   int i;
const char **nargv = xcalloc(sizeof(*nargv), argc + 3);
+   struct strbuf sb = STRBUF_INIT;
char *basedir_arg;
int nargc = 0;
 
@@ -65,11 +65,10 @@ int cmd_tar_tree(int argc, const char **argv, const char 
*prefix)
fprintf(stderr,
*** \git tar-tree\ is now deprecated.\n
*** Running \git archive\ instead.\n***);
-   for (i = 0; i  nargc; i++) {
-   fputc(' ', stderr);
-   sq_quote_print(stderr, nargv[i]);
-   }
-   fputc('\n', stderr);
+   sq_quote_argv(sb, nargv, 0);
+   strbuf_addch(sb, '\n');
+   fputs(sb.buf, stderr);
+   strbuf_release(sb);
return cmd_archive(nargc, nargv, prefix);
 }
 
-- 
1.8.4.rc0.4.g4634265

--
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

Help using git subtree

2013-07-30 Thread Gabriel Jover


Dear all,

I am trying to avoid using git submodules and thus I am testing if git 
subtree fit my needs.

I have a set of sub-projects linked to a main-project.

Git subtree is very convenient to get all the sub-projects into the 
main-project directory tree

and to send back sub-project commits to the corresponding repository.
But I don't understand the work flow very well.

I can add a subtree with a given prefix and pull/push my commits to the 
repository,
but how do I do to pull/push commits from the main-project to the 
corresponding repository

avoiding sub-projects to be sent?

An other question I have is when will be subtree included in git packages.
I had to compile git from sources in order to get this feature.

Sincerely,

G. Jover-Manas

--
++
 Gabriel V. Jover Mañas
 Computing Division
 Cells / Alba Synchrotron  [http:/www.cells.es]
 Carretera BP 1413 de Cerdanyola-Sant Cugat, Km. 3.3
 E-08290 Cerdanyola del Valles (Barcelona), Spain
 E-mail: gjo...@cells.es
 Phone: +34 93 592 4471
 Office: ALBA P0 Of8/18
++

Si heu rebut aquest correu per error, us informo que pot contenir informació 
confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho 
comuniqueu al remitent i l'elimineu. Gràcies.
Si ha recibido este correo por error, le informo de que puede contener 
información confidencial  y privada y que está prohibido su uso. Le agradeceré 
que lo comunique a su remitente y lo elimine. Gracias.
If you have received  this e-mail by error, please note that it may contain 
confidential and private  information, therefore, the use of this information 
is strictly forbidden. Please inform the sender of the error and delete the 
information received. Thank you.

--
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: Help using git subtree

2013-07-30 Thread Fredrik Gustafsson

On Tue, Jul 30, 2013 at 11:32:22AM +0200, Gabriel Jover wrote:
 Dear all,
 
 I am trying to avoid using git submodules and thus I am testing if
 git subtree fit my needs.
 I have a set of sub-projects linked to a main-project.

Just out of curiosity, why are you trying to avoid submodules?

 
 Git subtree is very convenient to get all the sub-projects into the
 main-project directory tree
 and to send back sub-project commits to the corresponding repository.
 But I don't understand the work flow very well.

I haven't used subtree that much however how do you commit to upstream?
AFAIK every commit to the subtree must be done in the subtree branch and
then merged back to the superproject-branch with a subtree merge.

 
 I can add a subtree with a given prefix and pull/push my commits to
 the repository,
 but how do I do to pull/push commits from the main-project to the
 corresponding repository
 avoiding sub-projects to be sent?

You don't. A subtree is a subtree in your git-repo and is therefore a
part of your repository. When you're using subtree that project will be
a part of you project.

 
 An other question I have is when will be subtree included in git packages.
 I had to compile git from sources in order to get this feature.
 

git-subtree is a part of git, located in the contrib section. When your
software distribution choose to distribute that with your git-package
is not up to the git project to decide but something you need to ask
your package maintainer.

If you're asking when git-subtree will move from contrib/ to core git, I
don't know. That's a decision for Junio. I would argue that it shouldn't
because submodules can do everything that subtrees are good at and
git.git should focus on doing one solution well instead of doing the
same thing in two bad ways. However since git.git already uses a
subtree-like style for gitk, git-gui, etc.  it doesn't seem like Junio
agrees with me on this point.

 
 Si heu rebut aquest correu per error, us informo que pot contenir informació 
 confidencial i privada i que està prohibit el seu ús. Us agrairiem que ho 
 comuniqueu al remitent i l'elimineu. Gràcies.
 Si ha recibido este correo por error, le informo de que puede contener 
 información confidencial  y privada y que está prohibido su uso. Le 
 agradeceré que lo comunique a su remitente y lo elimine. Gracias.
 If you have received  this e-mail by error, please note that it may contain 
 confidential and private  information, therefore, the use of this information 
 is strictly forbidden. Please inform the sender of the error and delete the 
 information received. Thank you.

Is this really something to have in e-mails to a public list? Some lists
won't accept this type of e-mails, or simply just ignore you.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
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] editor: use canonicalized absolute path

2013-07-30 Thread Duy Nguyen

On Mon, Jul 29, 2013 at 07:56:58AM -0700, Junio C Hamano wrote:
  diff --git a/editor.c b/editor.c
  index 27bdecd..0abbd8d 100644
  --- a/editor.c
  +++ b/editor.c
  @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf 
  *buffer, const char *const *en
  return error(Terminal is dumb, but EDITOR unset);
   
  if (strcmp(editor, :)) {
  -   const char *args[] = { editor, path, NULL };
  +   const char *args[] = { editor, real_path(path), NULL };
 
 While I am not fundamentally opposed to add a workaround for bugs in
 a popular tool many people use, I am a bit uneasy about this change.
 
 For editors that are not broken, this could be an annoying
 regression, isn't it?  When the user asks What is the path of the
 file I am editing? to the editor (i.e. an equivalent of \C-x\C-b),
 the updated code will start spewing a long full-path from the root
 directory, while we used to give a relative path that is short,
 sweet and more in line with the context of user's work.
 
 Compared to not being able to edit, it may be a small price to pay
 for those who do need to suffer the broken editor, but the patch
 makes those who do not need this workaround to pay the price.
 

How about something like this? For standard setups, even if you have
symlink in cwd, it won't kick in because the given path is usually
.git/something (i.e. no .. component)

-- 8 --
diff --git a/editor.c b/editor.c
index 27bdecd..02bf42c 100644
--- a/editor.c
+++ b/editor.c
@@ -37,10 +37,17 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
return error(Terminal is dumb, but EDITOR unset);
 
if (strcmp(editor, :)) {
+   char cwd[PATH_MAX];
const char *args[] = { editor, path, NULL };
struct child_process p;
int ret, sig;
 
+   /* emacs workaround */
+   if (getcwd(cwd, PATH_MAX) 
+   strcmp(real_path(cwd), cwd) 
+   strstr(path, ../))
+   args[1] = real_path(path);
+
memset(p, 0, sizeof(p));
p.argv = args;
p.env = env;
-- 8--
--
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] editor: use canonicalized absolute path

Duy Nguyen pclo...@gmail.com writes:

 The idea is the same, but my patch is a bit different (use of realpath
 instead of real_path, I didn't remember git has real_path). I'm fine
 with Ram being the author.

Thanks, both of you, for clarification.

 Compared to not being able to edit, it may be a small price to pay
 for those who do need to suffer the broken editor, but the patch
 makes those who do not need this workaround to pay the price.

 Does looking at the edited file's path happen often? I have never done
 that before. I ask because in order to avoid the price for those
 users, the code could get a little bit more complicated (detecting if
 the given relative path traverse backward outside a symlink..). To me
 it seems like a good trade off in favor of simpler code.

Yeah, I was being my usual cautious self, as somebody has to play
that role.  I think the code as-is would be an OK trade off.
--
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 3/3] config: --get-urlmatch

Jeff King p...@peff.net writes:

 Ah, I missed that you could leave key empty.

Yes, the general syntax is

git config [--type] --get-urlmatch section[.key] url

and giving section without a specific key would list all the
variables in the section that apply to url.

This is why we should do documentation at some point before
publishing a patch series; sorry 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

Re: [PATCH] contrib/subtree: Fix make install target

Michal Sojka sojk...@fel.cvut.cz writes:

 If the libexec directory doesn't exist, git-subtree gets installed as
 $prefix/share/libexec/git-core file. This patch creates the directory
 before installing git-subtree file into it.

 Signed-off-by: Michal Sojka sojk...@fel.cvut.cz
 ---

Will queue, as it is obviously correct.  Thanks.

I however wish somebody steps up to start properly maintaining this
part of the contrib ;-).

  contrib/subtree/Makefile | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile
 index b507505..435b2de 100644
 --- a/contrib/subtree/Makefile
 +++ b/contrib/subtree/Makefile
 @@ -30,6 +30,7 @@ $(GIT_SUBTREE): $(GIT_SUBTREE_SH)
  doc: $(GIT_SUBTREE_DOC)
  
  install: $(GIT_SUBTREE)
 + $(INSTALL) -d -m 755 $(DESTDIR)$(libexecdir)
   $(INSTALL) -m 755 $(GIT_SUBTREE) $(DESTDIR)$(libexecdir)
  
  install-doc: install-man
--
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] Documentation: fix typos in man pages

2013-07-30 Thread Marc Branchaud

On 13-07-29 05:15 PM, Øystein Walle wrote:
Signed-off-by: Øystein Walle oys...@gmail.com
---
I thought I'd take part in the typo fixing frenzy :)

I have some other potential typos lines up. Right now the docs refer to both
'filesystem' and 'file system', as well as both 'testsuite' and 'test suite'.
I
think words like these are generally split in English but I'm not sure.

I generally prefer to see the spaces in these words, otherwise it starts to
look more like German.

But of course English is full of exceptions...

There are also some words that I think look better with with a dash, e.g.
'trade-off'. Should I just send these as a patch too instead of jabbering on
about it?

I'm indifferent to that. I guess it depends on the context, so seeing the
patch would help.

I personally don't have a lot of time to investigate the nuances of English.
However, I desperately hope this list can avoid any linguistic flame wars.
In that spirit, I suggest that anyone posting an orthographic patch (i.e. for
something that isn't an obvious spelling mistake) could helpfully include a
link or two to an explanation of the reasoning for the change. Especially
for folks who aren't native English speakers, this could help avoid a lot of
back-and-forth.

One general source I've found is the English StackExchange:
http://english.stackexchange.com/

diff --git a/Documentation/git-check-ignore.txt
b/Documentation/git-check-ignore.txt
index d2df487..5354301 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -35,7 +35,7 @@ OPTIONS
Read file names from stdin instead of from the command-line.

-z::
- The output format is modified to be machine-parseable (see
+ The output format is modified to be machine-parsable (see

I believe this is a US/UK nuance. As I've recently stated, I think this kind
of change isn't all that helpful as we're likely to see some well-intentioned
person switch it back sometime in the future. If the git project could
choose an official English dialect it would go a long way towards mitigating
such churn.

below). If `--stdin` is also given, input paths are separated
with a NUL character instead of a linefeed character.

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 450f158..3865658 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -213,7 +213,7 @@ objects from the source repository into a pack in the
cloned repository.
--separate-git-dir=git dir::
Instead of placing the cloned repository where it is supposed
to be, place the cloned repository at the specified directory,
- then make a filesytem-agnostic Git symbolic link to there.
+ then make a filesystem-agnostic Git symbolic link to there.
The result is Git repository can be separated from working
tree.

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 223f731..a3283e1 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -189,7 +189,7 @@ Git configuration files in that directory are readable by
`user`.
service by exiting with a non-zero status (or to allow it by
exiting with a zero status). It can also look at the $REMOTE_ADDR
and $REMOTE_PORT environment variables to learn about the
- requestor when making this decision.
+ requester when making this decision.

Although I prefer the -or form for this word, this really is one of English's
vague areas. Some words that end with -st definitely take the -er suffix
(tester, jester) but others take the -or suffix (investor). A bit of
Googling also gave no firm result[1][2].

So I think this change is neither good nor bad. However, like the
UK/US-isms, I wonder if there's some way to avoid people changing this back
and forth. But I don't think simply choosing a dialect will help here.

[1]
http://english.stackexchange.com/questions/29254/whats-the-difference-between-requester-and-requestor
[2] http://www.spelling.hemscott.net/ends4.html

+
The external command can optionally write a single line to its
standard output to be sent to the requestor as an error message when
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 78d6d50..fe42bf6 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -39,7 +39,7 @@ directories. This behavior can be forced by --no-index.
commit relative to the named commit. Typically you
would want comparison

Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf

Ramkumar Ramachandra artag...@gmail.com writes:

 While going through the for-each-ref-pretty series that Duy and I were
 developing, I noticed that this cleanup was independent and good
 as-it-is.

 So here it is.

You always can first allocate a piece of memory and write into it
instead of writing things out directly.  The patch shows it _can_ be
done, but that is not a news.

And such a change is hardly a clean-up.  It just wastes more
memory you do not have to waste, in order to do what you are doing.

When there is a reason why you need an in-memory representation,
this change starts to become the first step refactoring for an
enhancement.

 Nguyễn Thái Ngọc Duy (1):
   for-each-ref, quote: convert *_quote_print - *_quote_buf

The log message for this one explains it very well.  This change by
itself is not useful, but it will become necessary once you start
needing to access an in-memory 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: [PATCH] Documentation: fix typos in man pages

Marc Branchaud marcn...@xiplink.com writes:

 I personally don't have a lot of time to investigate the nuances of English.
 However, I desperately hope this list can avoid any linguistic flame wars.
 In that spirit, I suggest that anyone posting an orthographic patch (i.e. for
 something that isn't an obvious spelling mistake) could helpfully include a
 link or two to an explanation of the reasoning for the change.  Especially
 for folks who aren't native English speakers, this could help avoid a lot of
 back-and-forth.

 One general source I've found is the English StackExchange:
   http://english.stackexchange.com/

I'd rather not to see any change that turns one accepted form into
another accepted form at all (like parseable vs parsable in this
patch).  For that purpose, asking What is parseable to Google and
seeing if there is a hit is good enough ;-)

I'd of course welcome genuine typofixes, like s/filesytem/filesystem/.

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

[PATCH] Specify UK English for the documentation source files.

2013-07-30 Thread Marc Branchaud

This will hopefully avoid questions over which spelling and grammar should
be used.  Translators are of course free to create localizations for other
English dialects.

Signed-off-by: Marc Branchaud marcn...@xiplink.com
---

Although I'm Canadian I figured en_CA would be a little too parochial.  I
don't have a strong preference for en_UK over en_US though.

On 13-07-30 11:05 AM, Junio C Hamano wrote: 
 I'd rather not to see any change that turns one accepted form into
 another accepted form at all (like parseable vs parsable in this
 patch).  For that purpose, asking What is parseable to Google and
 seeing if there is a hit is good enough ;-)

I think choosing a dialect for git will help mitigate such changes.  But
perhaps it's a bit too draconian.

M.


 Documentation/CodingGuidelines | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 559d5f9..43af82e 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -242,6 +242,9 @@ Writing Documentation:
  processed into HTML and manpages (e.g. git.html and git.1 in the
  same directory).
 
+ The human lanuage of the documentation source files is UK English (en_UK).
+ Please follow UK English norms for spelling and grammar.
+
  Every user-visible change should be reflected in the documentation.
  The same general rule as for code applies -- imitate the existing
  conventions.  A few commented examples follow to provide reference
-- 
1.8.3.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 0/3] Remove sq_quote_print() in favor of *_buf

Junio C Hamano gits...@pobox.com writes:

 Ramkumar Ramachandra artag...@gmail.com writes:

 While going through the for-each-ref-pretty series that Duy and I were
 developing, I noticed that this cleanup was independent and good
 as-it-is.

 So here it is.

 You always can first allocate a piece of memory and write into it
 instead of writing things out directly.  The patch shows it _can_ be
 done, but that is not a news.

 And such a change is hardly a clean-up.  It just wastes more
 memory you do not have to waste, in order to do what you are doing.

 When there is a reason why you need an in-memory representation,
 this change starts to become the first step refactoring for an
 enhancement.

Having said all that, the patch texts all look OK, so I'll queue
them with updated log messages.  It was the usual me reacting to
unjustified value judgement made in log messages and cover letters.

It would save me a lot of work if people stopped doing that and
instead stuck to facts.  For example, between print_to_buf() and
print_to_stdout(), the former is *not* necessarily nicer.  It is
more flexible but that flexibility comes with a price, and the
caller needs a demonstrative need for that flexibility to justify
the cost.

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: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure

Brandon Casey bca...@nvidia.com writes:

 From: Brandon Casey draf...@gmail.com

 When the number of open packs exceeds pack_max_fds, unuse_one_window()
 is called repeatedly to attempt to release the least-recently-used
 pack windows, which, as a side-effect, will also close a pack file
 after closing its last open window.  If a pack file has been opened,
 but no windows have been allocated into it, it will never be selected
 by unuse_one_window() and hence its file descriptor will not be
 closed.  When this happens, git may exceed the number of file
 descriptors permitted by the system.

An interesting find.  The patch from a cursory look reads OK.

Thanks.

 This is not likely to occur during upload-pack since upload-pack
 reads each object from the pack so that it can peel tags and
 advertise the exposed object.

Another interesting find.  Perhaps there is a room for improvements,
as packed-refs file knows what objects the tags peel to?  I vaguely
recall Peff was actively reducing the object access during ref
enumeration in not so distant past...
--
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

ANNOUNCE: git-integration -- Easily manage integration branches

2013-07-30 Thread John Keeping

I wrote this script a few months ago and have been using it pretty much
daily since then, so I figure it's time to see if anyone else finds it
useful...

git-integration [1] is a script to help manage integration branches in
Git.  By defining a base point and a set of branches to be merged to
form the integration branch, git-integration lets you easily rebuild an
integration branch when anything in it changes, as well as showing you
the status of all of the branches in the integration branch.

For example, the instruction sheet for git-integration's pu branch
recently looked like this:

base master

merge make-clean

  Add a clean target to the makefile.

merge version

  Support for --version option.

  N.B. this builds on make-clean.

merge skip-option

  Needs more work to be able to handle branch not found.

This tells git-integration to base the pu branch on master and merge
the make-clean, version and skip-option branches in.  The comments
following the merge instructions are added to the commit message for
the corresponding merge commit.  When I want to rebuild the pu branch
I simply do:

$ git integration --rebuild pu

To change the contents of the branch, I either edit the instruction
sheet manually:

$ git integration --edit pu

or quickly add a new branch from the command line:

$ git integration --add my-new-branch pu

In fact, I can combine these to get the benefit of bash-completion on
the branch name and the ability to edit the instruction sheet - when
multiple commands are specified, git-integration performs each of them
in a sensible order, described in the manpage [2].


[1] http://johnkeeping.github.io/git-integration/
[2] http://johnkeeping.github.io/git-integration/git-integration.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] Specify UK English for the documentation source files.

2013-07-30 Thread Fredrik Gustafsson

On Tue, Jul 30, 2013 at 11:11:54AM -0400, Marc Branchaud wrote:
 This will hopefully avoid questions over which spelling and grammar should
 be used.  Translators are of course free to create localizations for other
 English dialects.
 
 Signed-off-by: Marc Branchaud marcn...@xiplink.com
 ---
 
 Although I'm Canadian I figured en_CA would be a little too parochial.  I
 don't have a strong preference for en_UK over en_US though.
 

Since the settings (for example color) use en_US, I suggest _not_ to
specify en_UK as documentation language. Either set en_US or rather
don't set anything at all but go after google correct. That's the most
populair alternative. For example colour gets 422 000 000 hits and color
gets 2 260 000 000 hits. We should use color even if it's not en_UK.

It's a more pragmatic approach and most git users aren't native english
speakers anyway.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
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] imap-send: use Apple's Security framework for base64 encoding

David Aguilar dav...@gmail.com writes:

 From: Jeremy Huddleston jerem...@apple.com

 Use Apple's supported functions for base64 encoding instead
 of the deprecated OpenSSL functions.

 Signed-off-by: Jeremy Huddleston jerem...@apple.com
 Signed-off-by: David Aguilar dav...@gmail.com
 ---
 This version moves the tricky #ifdefs into git-compat-util.h

Nice.  I however wonder if we can kick the inlines that are
irrelevant to most people out further.  For example, would the
following be an improvement?

-- 8 --
From: Jeremy Huddleston jerem...@apple.com
Date: Mon, 29 Jul 2013 18:28:30 -0700
Subject: [PATCH] imap-send: use Apple's Security framework for base64 encoding

Use Apple's supported functions for base64 encoding instead
of the deprecated OpenSSL functions.

Signed-off-by: Jeremy Huddleston jerem...@apple.com
Signed-off-by: David Aguilar dav...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Makefile |  1 +
 compat/apple-common-crypto.h | 86 
 git-compat-util.h| 11 ++
 imap-send.c  | 14 
 4 files changed, 98 insertions(+), 14 deletions(-)
 create mode 100644 compat/apple-common-crypto.h

diff --git a/Makefile b/Makefile
index 5e7cadf..dddf49b 100644
--- a/Makefile
+++ b/Makefile
@@ -1398,6 +1398,7 @@ ifdef PPC_SHA1
LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
+   LIB_4_CRYPTO += -framework Security -framework CoreFoundation
COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
SHA1_HEADER = CommonCrypto/CommonDigest.h
 else
diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h
new file mode 100644
index 000..c8b9b0e
--- /dev/null
+++ b/compat/apple-common-crypto.h
@@ -0,0 +1,86 @@
+/* suppress inclusion of conflicting openssl functions */
+#define OPENSSL_NO_MD5
+#define HEADER_HMAC_H
+#define HEADER_SHA_H
+#include CommonCrypto/CommonHMAC.h
+#define HMAC_CTX CCHmacContext
+#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len)
+#define HMAC_Update CCHmacUpdate
+#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash)
+#define HMAC_CTX_cleanup(ignore)
+#define EVP_md5(...) kCCHmacAlgMD5
+#if __MAC_OS_X_VERSION_MIN_REQUIRED = 1070
+#define APPLE_LION_OR_NEWER
+#include Security/Security.h
+/* Apple's TYPE_BOOL conflicts with config.c */
+#undef TYPE_BOOL
+#endif
+
+#ifdef APPLE_LION_OR_NEWER
+#define git_CC_error_check(pattern, err) \
+   do { \
+   if (err) { \
+   die(pattern, (long)CFErrorGetCode(err)); \
+   } \
+   } while(0)
+
+#define EVP_EncodeBlock git_CC_EVP_EncodeBlock
+static inline int git_CC_EVP_EncodeBlock(unsigned char *out,
+   const unsigned char *in, int inlen)
+{
+   CFErrorRef err;
+   SecTransformRef encoder;
+   CFDataRef input, output;
+   CFIndex length;
+
+   encoder = SecEncodeTransformCreate(kSecBase64Encoding, err);
+   git_CC_error_check(SecEncodeTransformCreate failed: %ld, err);
+
+   input = CFDataCreate(kCFAllocatorDefault, in, inlen);
+   SecTransformSetAttribute(encoder, kSecTransformInputAttributeName,
+   input, err);
+   git_CC_error_check(SecTransformSetAttribute failed: %ld, err);
+
+   output = SecTransformExecute(encoder, err);
+   git_CC_error_check(SecTransformExecute failed: %ld, err);
+
+   length = CFDataGetLength(output);
+   CFDataGetBytes(output, CFRangeMake(0, length), out);
+
+   CFRelease(output);
+   CFRelease(input);
+   CFRelease(encoder);
+
+   return (int)strlen((const char *)out);
+}
+
+#define EVP_DecodeBlock git_CC_EVP_DecodeBlock
+static int inline git_CC_EVP_DecodeBlock(unsigned char *out,
+   const unsigned char *in, int inlen)
+{
+   CFErrorRef err;
+   SecTransformRef decoder;
+   CFDataRef input, output;
+   CFIndex length;
+
+   decoder = SecDecodeTransformCreate(kSecBase64Encoding, err);
+   git_CC_error_check(SecEncodeTransformCreate failed: %ld, err);
+
+   input = CFDataCreate(kCFAllocatorDefault, in, inlen);
+   SecTransformSetAttribute(decoder, kSecTransformInputAttributeName,
+   input, err);
+   git_CC_error_check(SecTransformSetAttribute failed: %ld, err);
+
+   output = SecTransformExecute(decoder, err);
+   git_CC_error_check(SecTransformExecute failed: %ld, err);
+
+   length = CFDataGetLength(output);
+   CFDataGetBytes(output, CFRangeMake(0, length), out);
+
+   CFRelease(output);
+   CFRelease(input);
+   CFRelease(decoder);
+
+   return (int)strlen((const char *)out);
+}
+#endif /* APPLE_LION_OR_NEWER */
diff --git a/git-compat-util.h b/git-compat-util.h
index e955bb5..6ebb029 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -127,6 +127,17 @@
 #else
 #include poll.h
 #endif
+
+#ifndef NO_OPENSSL
+#ifdef APPLE_COMMON_CRYPTO
+#include compat/apple-common-crypto.h
+#else

Re: [PATCH] Specify UK English for the documentation source files.

Marc Branchaud marcn...@xiplink.com writes:

 This will hopefully avoid questions over which spelling and grammar should
 be used.  Translators are of course free to create localizations for other
 English dialects.

 Signed-off-by: Marc Branchaud marcn...@xiplink.com
 ---

 Although I'm Canadian I figured en_CA would be a little too parochial.  I
 don't have a strong preference for en_UK over en_US though.

If we were to specify one, I would have to say we should standardise
on en_US.  Like it or not, that is the lingua franca in the CS world
(I'd also admit that most of the extra 'u' in words like behaviour
are my typoes, not even coming from a conscious decision to choose
en_UK).

I'd appreciate typofix patches to address genuine typoes like
s/filesytem/filesystem/ done separately, so that we do not even
have to get into language bikeshedding.  Even if we did decide to
stick to en_US, the typofix patches and en_US patches should be
separate to ease reviews.
--
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-integration -- Easily manage integration branches

John Keeping j...@keeping.me.uk writes:

 I wrote this script a few months ago and have been using it pretty much
 daily since then, so I figure it's time to see if anyone else finds it
 useful...

 git-integration [1] is a script to help manage integration branches in
 Git.  By defining a base point and a set of branches to be merged to
 form the integration branch, git-integration lets you easily rebuild an
 integration branch when anything in it changes, as well as showing you
 the status of all of the branches in the integration branch.

 For example, the instruction sheet for git-integration's pu branch
 recently looked like this:

   base master

   merge make-clean

 Add a clean target to the makefile.

   merge version

 Support for --version option.

 N.B. this builds on make-clean.

   merge skip-option

 Needs more work to be able to handle branch not found.

 This tells git-integration to base the pu branch on master and merge
 the make-clean, version and skip-option branches in.  The comments
 following the merge instructions are added to the commit message for
 the corresponding merge commit.  When I want to rebuild the pu branch
 I simply do:

   $ git integration --rebuild pu

 To change the contents of the branch, I either edit the instruction
 sheet manually:

   $ git integration --edit pu

 or quickly add a new branch from the command line:

   $ git integration --add my-new-branch pu

 In fact, I can combine these to get the benefit of bash-completion on
 the branch name and the ability to edit the instruction sheet - when
 multiple commands are specified, git-integration performs each of them
 in a sensible order, described in the manpage [2].


 [1] http://johnkeeping.github.io/git-integration/
 [2] http://johnkeeping.github.io/git-integration/git-integration.html

Interesting.

Would it help me to replay evil merges I previously made and avoid
necessity to write merge log messages repeatedly?

In short, can I replace my Meta/Reintegrate and Meta/cook with this
(see Documentation/howto/maintain-git.txt)?

--
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-integration -- Easily manage integration branches

2013-07-30 Thread John Keeping

On Tue, Jul 30, 2013 at 09:45:49AM -0700, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk writes:
 
  I wrote this script a few months ago and have been using it pretty much
  daily since then, so I figure it's time to see if anyone else finds it
  useful...
 
  git-integration [1] is a script to help manage integration branches in
  Git.  By defining a base point and a set of branches to be merged to
  form the integration branch, git-integration lets you easily rebuild an
  integration branch when anything in it changes, as well as showing you
  the status of all of the branches in the integration branch.
 
  For example, the instruction sheet for git-integration's pu branch
  recently looked like this:
 
  base master
 
  merge make-clean
 
Add a clean target to the makefile.
 
  merge version
 
Support for --version option.
 
N.B. this builds on make-clean.
 
  merge skip-option
 
Needs more work to be able to handle branch not found.
 
  This tells git-integration to base the pu branch on master and merge
  the make-clean, version and skip-option branches in.  The comments
  following the merge instructions are added to the commit message for
  the corresponding merge commit.  When I want to rebuild the pu branch
  I simply do:
 
  $ git integration --rebuild pu
 
  To change the contents of the branch, I either edit the instruction
  sheet manually:
 
  $ git integration --edit pu
 
  or quickly add a new branch from the command line:
 
  $ git integration --add my-new-branch pu
 
  In fact, I can combine these to get the benefit of bash-completion on
  the branch name and the ability to edit the instruction sheet - when
  multiple commands are specified, git-integration performs each of them
  in a sensible order, described in the manpage [2].
 
 
  [1] http://johnkeeping.github.io/git-integration/
  [2] http://johnkeeping.github.io/git-integration/git-integration.html
 
 Interesting.
 
 Would it help me to replay evil merges I previously made and avoid
 necessity to write merge log messages repeatedly?

Currently it does nothing beyond having the ability to continue
automatically if rerere manages to resolve all conflicts (disabled by
default).  There is no equivalent of your refs/merge-fix/ feature,
although I think I might add one soon ;-).

Since the commit messages for the merge commits come from the
instruction sheet, it does avoid the need to write them repeatedly - if
you want to change the merge message you can simply update the
instruction sheet and rebuild.

 In short, can I replace my Meta/Reintegrate and Meta/cook with this
 (see Documentation/howto/maintain-git.txt)?

It performs the same basic function as those scripts, but it's quite a
lot simpler and hasn't been designed for the git.git workflow, so I
don't think it's suitable for replacing your existing scripts.

If I were starting from scratch and attempting to implement the git.git
workflow on top of git-integration, I think I would make
whats-cooking.txt a build artifact generated from the instruction sheet
for pu.  This would require some new commands to be added to
git-integration's instruction sheet to let it assign sections to
branches, but ought to be possible.  I expect there would be some
subtleties though - certainly git-integration's --status output does
not handle all of the cases the Meta/cook does, not least because it
only compares against a single base branch.
--
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] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

2013-07-30 Thread Stefan Beller

As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),
the OPT_BOOLEAN was deprecated.
While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or
the OPT_COUNTUP to keep existing behavior, this commit is actually a
bug fix!

In line 499 we have:
if (list + delete + verify  1)
usage_with_options(git_tag_usage, options);
Now if we give one of the options twice, we'll get the usage information.
(i.e. 'git tag --verify --verify tagname' and
'git --delete --delete tagname' yield usage information and do not
do the intended command.)

This could have been fixed by rewriting the line to
if (!!list + !!delete + !!verify  1)
usage_with_options(git_tag_usage, options);
or as it happened in this patch by having the parameters not
counting up for each occurrence, but the OPT_BOOL just setting the
variables to either 0 if the option is not given or 1 if the option is
given multiple times.

However we could discuss if the negated options do make sense here, or if
we don't want to allow them here, as this seems valid (before and after
this patch):
git tag --no-verify --delete tagname

Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
 builtin/tag.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index b3942e4..d155c9d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -442,12 +442,12 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
struct option options[] = {
-   OPT_BOOLEAN('l', list, list, N_(list tag names)),
+   OPT_BOOL('l', list, list, N_(list tag names)),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
N_(print n lines of each tag message),
PARSE_OPT_OPTARG, NULL, 1 },
-   OPT_BOOLEAN('d', delete, delete, N_(delete tags)),
-   OPT_BOOLEAN('v', verify, verify, N_(verify tags)),
+   OPT_BOOL('d', delete, delete, N_(delete tags)),
+   OPT_BOOL('v', verify, verify, N_(verify tags)),
 
OPT_GROUP(N_(Tag creation options)),
OPT_BOOL('a', annotate, annotate,
@@ -455,7 +455,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', message, msg, N_(message),
 N_(tag message), parse_msg_arg),
OPT_FILENAME('F', file, msgfile, N_(read message from 
file)),
-   OPT_BOOLEAN('s', sign, opt.sign, N_(annotated and 
GPG-signed tag)),
+   OPT_BOOL('s', sign, opt.sign, N_(annotated and GPG-signed 
tag)),
OPT_STRING(0, cleanup, cleanup_arg, N_(mode),
N_(how to strip spaces and #comments from message)),
OPT_STRING('u', local-user, keyid, N_(key id),
-- 
1.8.4.rc0.1.g8f6a3e5

--
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 5/3] revert most of the http_options() change

2013-07-30 Thread Kyle J. McKay


On Jul 29, 2013, at 19:13, Junio C Hamano wrote:


With the previous preparation step, the earlier 1bb6 (config:
add support for http.url.* settings, 2013-07-21) that introduced
many repeated changes:

   -if (!strcmp(http.key, var)) {
   +if (!strcmp(key, key)) {
   +   if (match_is_shorter(..., OPT_KEY_NAME))
   +   return 0;
   ... original processing for KEY_NAME ...
}

can be reverted.

This allows us to also get rid of the repeating yourself to the
death enum http_option_type, and new code (like db/http-savecookies
patch) that wants to add a new configuration to the http.* subsystem
does not have to conflict with http.urlpattern.variable series at
all.

This is more-or-less how I want the endgame to look like. Not even
compile tested, but it is getting late so I'll show it to the list
in case people may want to play with it and polish it while I am
away for the night ;-).

If people can agree that this is going in the right direction,
perhaps we should rebuild Kyle's series without detouring of adding
and then yanking what 1bb6 (config: add support for http.url.*
settings, 2013-07-21) did in the next cycle.

http.c   | 181  
+--

test-url-normalize.c |   9 ++-
2 files changed, 39 insertions(+), 151 deletions(-)

diff --git a/http.c b/http.c
index a91a00b..c7f513b 100644
--- a/http.c
+++ b/http.c

[...]
@@ -469,15 +342,23 @@ void http_init(struct remote *remote, const  
char *url, int proactive_auth)

{
char *low_speed_limit;
char *low_speed_time;
-   struct url_info info;
+   char *normalized_url;
+   struct urlmatch_config config = { STRING_LIST_INIT_DUP };
+
+   config.fn = http_options;
+   config.cascade_fn = git_default_config;
+   config.item_alloc = NULL;
+   config.item_clear = NULL;
+   config.cb = NULL;


Missing this:
+   config.section = http;


[...]

diff --git a/test-url-normalize.c b/test-url-normalize.c
index 0b809eb..fab94e5 100644
--- a/test-url-normalize.c
+++ b/test-url-normalize.c
@@ -15,8 +15,15 @@ static int run_http_options(const char *file,
{
struct strbuf opt_lc;
size_t i, len;
+   struct urlmatch_config config = { STRING_LIST_INIT_DUP };

-   if (git_config_with_options(http_options, (void *)info, file, 0))
+   config.fn = http_options;
+   config.cascade_fn = git_default_config;
+   config.item_alloc = NULL;
+   config.item_clear = NULL;
+   config.cb = NULL;



Missing this:
+   config.section = http;

Without these it segfaults running the tests.

However, now the tests that actually check the results using config  
files are failing:



# failed 1 among 11 test(s)


I'm looking at it now to see if I can fix the problem.
--
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 3/3] config: --get-urlmatch

2013-07-30 Thread Kyle J. McKay


On Jul 29, 2013, at 15:49, Junio C Hamano wrote:


git config --get-urlmatch $section[.$variable] $url is a way to
learn what the configured value for $section.$variable is for the
given URL, using the logic introduced by the http.url.config
topic.  In addition to $section.$variable, entries in the
configuration file(s) that match $section.urlpattern.$variable are
looked up and the one with urlpattern that matches the given $url
the best is used to answer the query.

This can still be further refactored to remove code from  
http_options()

in http.c, I think.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/config.c | 141 ++ 
+

1 file changed, 141 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index 12c5073..c1d32ae 100644
--- a/builtin/config.c
+++ b/builtin/config.c

[...]

+static int get_urlmatch(const char *var, const char *url)
+{
+   const char *section_tail;
+   struct string_list_item *item;
+   struct urlmatch_collect collect = { STRING_LIST_INIT_DUP };
+
+   if (!url_normalize(url, collect.url))
+   die(collect.url.err);


The value now stored in collect.url.url is never freed.


+
+   section_tail = strchr(var, '.');
+   if (section_tail) {
+   collect.section = xmemdupz(var, section_tail - var);
+   collect.key = strrchr(var, '.') + 1;
+   show_keys = 0;
+   } else {
+   collect.section = var;
+   collect.key = NULL;
+   show_keys = 1;
+   }
+
+   git_config_with_options(urlmatch_collect, collect,
+   given_config_file, respect_includes);
+
+   for_each_string_list_item(item, collect.vars) {
+   struct urlmatch_item *matched = item-util;
+   struct strbuf key = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
+
+   strbuf_addstr(key, collect.section);
+   strbuf_addch(key, '.');
+   strbuf_addstr(key, item-string);
+   format_config(buf, key.buf,
+ matched-value_is_null ? NULL : 
matched-value.buf);
+   fwrite(buf.buf, 1, buf.len, stdout);
+   strbuf_release(key);
+   strbuf_release(buf);
+
+   strbuf_release(matched-value);
+   }
+   string_list_clear(collect.vars, 1);


Needs something like this here:

+   free(collect.url.url);


+
+   /*
+* section name may have been copied to replace the dot, in which
+* case it needs to be freed.  key name is either NULL (e.g. 'http'
+* alone) or points into var (e.g. 'http.savecookies'), and we do
+* not own the storage.
+*/
+   if (collect.section != var)
+   free((void *)collect.section);
+   return 0;
+}


Still needed after 4/3 except it's now config.url.url instead.
--
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] imap-send: use Apple's Security framework for base64 encoding

2013-07-30 Thread David Aguilar

On Tue, Jul 30, 2013 at 8:54 AM, Junio C Hamano gits...@pobox.com wrote:
 David Aguilar dav...@gmail.com writes:

 From: Jeremy Huddleston jerem...@apple.com

 Use Apple's supported functions for base64 encoding instead
 of the deprecated OpenSSL functions.

 Signed-off-by: Jeremy Huddleston jerem...@apple.com
 Signed-off-by: David Aguilar dav...@gmail.com
 ---
 This version moves the tricky #ifdefs into git-compat-util.h

 Nice.  I however wonder if we can kick the inlines that are
 irrelevant to most people out further.  For example, would the
 following be an improvement?

Yes, IMO that is nicer.  It keeps all of the Apple specifics neatly tucked away.
Thanks


 -- 8 --
 From: Jeremy Huddleston jerem...@apple.com
 Date: Mon, 29 Jul 2013 18:28:30 -0700
 Subject: [PATCH] imap-send: use Apple's Security framework for base64 encoding

 Use Apple's supported functions for base64 encoding instead
 of the deprecated OpenSSL functions.

 Signed-off-by: Jeremy Huddleston jerem...@apple.com
 Signed-off-by: David Aguilar dav...@gmail.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  Makefile |  1 +
  compat/apple-common-crypto.h | 86 
 
  git-compat-util.h| 11 ++
  imap-send.c  | 14 
  4 files changed, 98 insertions(+), 14 deletions(-)
  create mode 100644 compat/apple-common-crypto.h

 diff --git a/Makefile b/Makefile
 index 5e7cadf..dddf49b 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -1398,6 +1398,7 @@ ifdef PPC_SHA1
 LIB_H += ppc/sha1.h
  else
  ifdef APPLE_COMMON_CRYPTO
 +   LIB_4_CRYPTO += -framework Security -framework CoreFoundation
 COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
 SHA1_HEADER = CommonCrypto/CommonDigest.h
  else
 diff --git a/compat/apple-common-crypto.h b/compat/apple-common-crypto.h
 new file mode 100644
 index 000..c8b9b0e
 --- /dev/null
 +++ b/compat/apple-common-crypto.h
 @@ -0,0 +1,86 @@
 +/* suppress inclusion of conflicting openssl functions */
 +#define OPENSSL_NO_MD5
 +#define HEADER_HMAC_H
 +#define HEADER_SHA_H
 +#include CommonCrypto/CommonHMAC.h
 +#define HMAC_CTX CCHmacContext
 +#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len)
 +#define HMAC_Update CCHmacUpdate
 +#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash)
 +#define HMAC_CTX_cleanup(ignore)
 +#define EVP_md5(...) kCCHmacAlgMD5
 +#if __MAC_OS_X_VERSION_MIN_REQUIRED = 1070
 +#define APPLE_LION_OR_NEWER
 +#include Security/Security.h
 +/* Apple's TYPE_BOOL conflicts with config.c */
 +#undef TYPE_BOOL
 +#endif
 +
 +#ifdef APPLE_LION_OR_NEWER
 +#define git_CC_error_check(pattern, err) \
 +   do { \
 +   if (err) { \
 +   die(pattern, (long)CFErrorGetCode(err)); \
 +   } \
 +   } while(0)
 +
 +#define EVP_EncodeBlock git_CC_EVP_EncodeBlock
 +static inline int git_CC_EVP_EncodeBlock(unsigned char *out,
 +   const unsigned char *in, int inlen)
 +{
 +   CFErrorRef err;
 +   SecTransformRef encoder;
 +   CFDataRef input, output;
 +   CFIndex length;
 +
 +   encoder = SecEncodeTransformCreate(kSecBase64Encoding, err);
 +   git_CC_error_check(SecEncodeTransformCreate failed: %ld, err);
 +
 +   input = CFDataCreate(kCFAllocatorDefault, in, inlen);
 +   SecTransformSetAttribute(encoder, kSecTransformInputAttributeName,
 +   input, err);
 +   git_CC_error_check(SecTransformSetAttribute failed: %ld, err);
 +
 +   output = SecTransformExecute(encoder, err);
 +   git_CC_error_check(SecTransformExecute failed: %ld, err);
 +
 +   length = CFDataGetLength(output);
 +   CFDataGetBytes(output, CFRangeMake(0, length), out);
 +
 +   CFRelease(output);
 +   CFRelease(input);
 +   CFRelease(encoder);
 +
 +   return (int)strlen((const char *)out);
 +}
 +
 +#define EVP_DecodeBlock git_CC_EVP_DecodeBlock
 +static int inline git_CC_EVP_DecodeBlock(unsigned char *out,
 +   const unsigned char *in, int inlen)
 +{
 +   CFErrorRef err;
 +   SecTransformRef decoder;
 +   CFDataRef input, output;
 +   CFIndex length;
 +
 +   decoder = SecDecodeTransformCreate(kSecBase64Encoding, err);
 +   git_CC_error_check(SecEncodeTransformCreate failed: %ld, err);
 +
 +   input = CFDataCreate(kCFAllocatorDefault, in, inlen);
 +   SecTransformSetAttribute(decoder, kSecTransformInputAttributeName,
 +   input, err);
 +   git_CC_error_check(SecTransformSetAttribute failed: %ld, err);
 +
 +   output = SecTransformExecute(decoder, err);
 +   git_CC_error_check(SecTransformExecute failed: %ld, err);
 +
 +   length = CFDataGetLength(output);
 +   CFDataGetBytes(output, CFRangeMake(0, length), out);
 +
 +   CFRelease(output);
 +   CFRelease(input);
 +   CFRelease(decoder);
 +
 +   return (int)strlen((const char *)out);
 +}
 +#endif /* APPLE_LION_OR_NEWER

Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

Stefan Beller stefanbel...@googlemail.com writes:

 As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),
 the OPT_BOOLEAN was deprecated.
 While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or
 the OPT_COUNTUP to keep existing behavior, this commit is actually a
 bug fix!

 In line 499 we have:
   if (list + delete + verify  1)
   usage_with_options(git_tag_usage, options);
 Now if we give one of the options twice, we'll get the usage information.
 (i.e. 'git tag --verify --verify tagname' and
 'git --delete --delete tagname' yield usage information and do not
 do the intended command.)

 This could have been fixed by rewriting the line to
   if (!!list + !!delete + !!verify  1)
   usage_with_options(git_tag_usage, options);
 or as it happened in this patch by having the parameters not
 counting up for each occurrence, but the OPT_BOOL just setting the
 variables to either 0 if the option is not given or 1 if the option is
 given multiple times.

Makes twisted sort of sense ;-).

 However we could discuss if the negated options do make sense
 here, or if we don't want to allow them here, as this seems valid
 (before and after this patch):

   git tag --no-verify --delete tagname

It probably does not.  As you hinted in your earlier patch, we may
want to introduce a only can set to true boolean used solely to
specify these things.  They are disguised as options, but are in
fact command operation modes that are often mutually exclusive.

For these operation modes that are mutually exclusive, there are
multiple possible implementations:

 * One OPT_BOOL_NONEG per option; the code ensures the mutual
   exclusion with if (list + delete + verify  1);

 * One OPT_BIT per option in a single variable; the code ensures the
   mutual exclusion with count_bits, which may be a lot more
   cumbersome;

 * OPT_SET_INT that updates a single variable to enum; instead of
   making it an error to give two conflicting modes, this would give
   us the last-one-wins rule.

Unlike usual options, we generally do not want the last-one-wins
semantics for command operation modes, I think.

Perhaps we would want something like this?

-- 8 --
Subject: [PATCH] parse-options: add OPT_CMDMODE()

This can be used to define a set of mutually exclusive command
mode options, and automatically catch use of more than one from
that set as an error.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 parse-options.c | 58 -
 parse-options.h |  3 +++
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index c2cbca2..62e9b1c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char 
**file)
*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));
 }
 
+static int opt_command_mode_error(const struct option *opt,
+ const struct option *all_opts,
+ int flags)
+{
+   const struct option *that;
+   struct strbuf message = STRBUF_INIT;
+   struct strbuf that_name = STRBUF_INIT;
+
+   /*
+* Find the other option that was used to set the variable
+* already, and report that this is not compatible with it.
+*/
+   for (that = all_opts; that-type != OPTION_END; that++) {
+   if (that == opt ||
+   that-type != OPTION_CMDMODE ||
+   that-value != opt-value ||
+   that-defval != *(int *)opt-value)
+   continue;
+
+   if (that-long_name)
+   strbuf_addf(that_name, --%s, that-long_name);
+   else
+   strbuf_addf(that_name, -%c, that-short_name);
+   strbuf_addf(message, : incompatible with %s, that_name.buf);
+   strbuf_release(that_name);
+   opterror(opt, message.buf, flags);
+   strbuf_release(message);
+   return -1;
+   }
+   return opterror(opt, : incompatible with something else, flags);
+}
+
 static int get_value(struct parse_opt_ctx_t *p,
-const struct option *opt, int flags)
+const struct option *opt,
+const struct option *all_opts,
+int flags)
 {
const char *s, *arg;
const int unset = flags  OPT_UNSET;
@@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p,
*(int *)opt-value = unset ? 0 : opt-defval;
return 0;
 
+   case OPTION_CMDMODE:
+   /*
+* Giving the same mode option twice, although is unnecessary,
+* is not a grave error, so let it pass.
+*/
+   if (*(int *)opt-value  *(int *)opt-value != opt-defval)
+   return opt_command_mode_error(opt,

Re: [PATCH 5/3] revert most of the http_options() change

2013-07-30 Thread Kyle J. McKay



On Jul 30, 2013, at 12:14, Kyle J. McKay wrote:


On Jul 29, 2013, at 19:13, Junio C Hamano wrote:


With the previous preparation step, the earlier 1bb6 (config:
add support for http.url.* settings, 2013-07-21) that introduced
many repeated changes:

  -if (!strcmp(http.key, var)) {
  +if (!strcmp(key, key)) {
  +   if (match_is_shorter(..., OPT_KEY_NAME))
  +   return 0;
  ... original processing for KEY_NAME ...
   }

can be reverted.

This allows us to also get rid of the repeating yourself to the
death enum http_option_type, and new code (like db/http-savecookies
patch) that wants to add a new configuration to the http.* subsystem
does not have to conflict with http.urlpattern.variable series at
all.

This is more-or-less how I want the endgame to look like. Not even
compile tested, but it is getting late so I'll show it to the list
in case people may want to play with it and polish it while I am
away for the night ;-).

If people can agree that this is going in the right direction,
perhaps we should rebuild Kyle's series without detouring of adding
and then yanking what 1bb6 (config: add support for http.url.*
settings, 2013-07-21) did in the next cycle.

http.c   | 181  
+--

test-url-normalize.c |   9 ++-
2 files changed, 39 insertions(+), 151 deletions(-)

diff --git a/http.c b/http.c
index a91a00b..c7f513b 100644
--- a/http.c
+++ b/http.c

[...]
@@ -469,15 +342,23 @@ void http_init(struct remote *remote, const  
char *url, int proactive_auth)

{
char *low_speed_limit;
char *low_speed_time;
-   struct url_info info;
+   char *normalized_url;
+   struct urlmatch_config config = { STRING_LIST_INIT_DUP };
+
+   config.fn = http_options;
+   config.cascade_fn = git_default_config;
+   config.item_alloc = NULL;
+   config.item_clear = NULL;
+   config.cb = NULL;


Missing this:
+   config.section = http;


[...]

diff --git a/test-url-normalize.c b/test-url-normalize.c
index 0b809eb..fab94e5 100644
--- a/test-url-normalize.c
+++ b/test-url-normalize.c
@@ -15,8 +15,15 @@ static int run_http_options(const char *file,
{
struct strbuf opt_lc;
size_t i, len;
+   struct urlmatch_config config = { STRING_LIST_INIT_DUP };


Also needs this:

+   memcpy(config.url, info, sizeof(struct url_info));


-   if (git_config_with_options(http_options, (void *)info, file, 0))
+   config.fn = http_options;
+   config.cascade_fn = git_default_config;
+   config.item_alloc = NULL;
+   config.item_clear = NULL;
+   config.cb = NULL;



Missing this:
+   config.section = http;

Without these it segfaults running the tests.

However, now the tests that actually check the results using config  
files are failing:



# failed 1 among 11 test(s)


I'm looking at it now to see if I can fix the problem.


And now all the t5200-url-normalize tests pass again.

FYI, I couldn't get the patches to apply against next or pu without  
some minor tweaks that were just conflict resolutions having to do  
with git_config_with_options changing its signature.

--
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 v3 0/5] Teach mv to move submodules

Here is my third iteration of this series.

Changes to v2 are:

- I resolved the conflict with Duy's pathspec series by replacing the
  use of common_prefix() with relative_path().

- I separated the functions checking for modified unstaged .gitmodules
  and staging the changes to that file into another commit, as they
  are used by both mv and rm.

- mv and rm now die with the message Please, stage your changes to
  .gitmodules or stash them to proceed instead of changing and
  staging a .gitmodules file containing other unstaged modifications.

- Man pages for mv and rm are updated to tell the user what they do
  with the gitlink and the .gitmodules file in case of submodules.

- Minor changes according to the last review (typos and a bit more
  efficient coding).

This series applies cleanly on current pu (and I also ran t3600 and
t7001 manually to make sure I don't hit the silent breakage my last
series showed when I ran the whole test suite).

Jens Lehmann (5):
  Teach mv to move submodules together with their work trees
  Teach mv to move submodules using a gitfile
  submodule.c: add .gitmodules staging helper functions
  Teach mv to update the path entry in .gitmodules for moved submodules
  rm: delete .gitmodules entry of submodules removed from the work tree

 Documentation/git-mv.txt   |  10 ++-
 Documentation/git-rm.txt   |   8 ++-
 builtin/mv.c   | 126 ++
 builtin/rm.c   |  19 +-
 submodule.c| 147 +
 submodule.h|   5 ++
 t/t3600-rm.sh  |  98 --
 t/t7001-mv.sh  | 128 +++
 t/t7400-submodule-basic.sh |  14 ++---
 t/t7610-mergetool.sh   |   6 +-
 10 files changed, 484 insertions(+), 77 deletions(-)

-- 
1.8.4.rc0.199.g7079aac


--
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 v3 1/5] Teach mv to move submodules together with their work trees

Currently the attempt to use git mv on a submodule errors out with:
  fatal: source directory is empty, source=src, destination=dest
The reason is that mv searches for the submodule with a trailing slash in
the index, which it doesn't find (because it is stored without a trailing
slash). As it doesn't find any index entries inside the submodule it
claims the directory would be empty even though it isn't.

Fix that by searching for the name without a trailing slash and continue
if it is a submodule. Then rename() will move the submodule work tree just
like it moves a file.

Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
 builtin/mv.c  | 99 +++
 t/t7001-mv.sh | 34 
 2 files changed, 86 insertions(+), 47 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 16ce99b..1d3ef63 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -118,55 +118,60 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
 lstat(dst, st) == 0)
bad = _(cannot move directory over file);
else if (src_is_dir) {
-   const char *src_w_slash = add_slash(src);
-   int len_w_slash = length + 1;
-   int first, last;
-
-   modes[i] = WORKING_DIRECTORY;
-
-   first = cache_name_pos(src_w_slash, len_w_slash);
-   if (first = 0)
-   die (_(Huh? %.*s is in index?),
-   len_w_slash, src_w_slash);
-
-   first = -1 - first;
-   for (last = first; last  active_nr; last++) {
-   const char *path = active_cache[last]-name;
-   if (strncmp(path, src_w_slash, len_w_slash))
-   break;
-   }
-   free((char *)src_w_slash);
-
-   if (last - first  1)
-   bad = _(source directory is empty);
-   else {
-   int j, dst_len;
-
-   if (last - first  0) {
-   source = xrealloc(source,
-   (argc + last - first)
-   * sizeof(char *));
-   destination = xrealloc(destination,
-   (argc + last - first)
-   * sizeof(char *));
-   modes = xrealloc(modes,
-   (argc + last - first)
-   * sizeof(enum 
update_mode));
+   int first = cache_name_pos(src, length);
+   if (first = 0) {
+   if (!S_ISGITLINK(active_cache[first]-ce_mode))
+   die (_(Huh? Directory %s is in index 
and no submodule?), src);
+   } else {
+   const char *src_w_slash = add_slash(src);
+   int last, len_w_slash = length + 1;
+
+   modes[i] = WORKING_DIRECTORY;
+
+   first = cache_name_pos(src_w_slash, 
len_w_slash);
+   if (first = 0)
+   die (_(Huh? %.*s is in index?),
+   len_w_slash, 
src_w_slash);
+
+   first = -1 - first;
+   for (last = first; last  active_nr; last++) {
+   const char *path = 
active_cache[last]-name;
+   if (strncmp(path, src_w_slash, 
len_w_slash))
+   break;
}
-
-   dst = add_slash(dst);
-   dst_len = strlen(dst);
-
-   for (j = 0; j  last - first; j++) {
-   const char *path =
-   active_cache[first + j]-name;
-   source[argc + j] = path;
-   destination[argc + j] =
-   prefix_path(dst, dst_len,
-   path + length + 1);
-   modes[argc + j] = INDEX;
+   free((char *)src_w_slash);
+
+   if (last - first  1)
+   bad = _(source directory

[PATCH v3 2/5] Teach mv to move submodules using a gitfile

When moving a submodule which uses a gitfile to point to the git directory
stored in .git/modules/name of the superproject two changes must be made
to make the submodule work: the .git file and the core.worktree setting
must be adjusted to point from work tree to git directory and back.

Achieve that by remembering which submodule uses a gitfile by storing the
result of read_gitfile() of each submodule. If that is not NULL the new
function connect_work_tree_and_git_dir() is called after renaming the
submodule's work tree which updates the two settings to the new values.

Extend the man page to inform the user about that feature (and while at it
change the description to not talk about a script anymore, as mv is a
builtin for quite some time now).

Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
 Documentation/git-mv.txt |  8 +++-
 builtin/mv.c | 19 +++
 submodule.c  | 31 +++
 submodule.h  |  1 +
 t/t7001-mv.sh| 19 +++
 5 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index e93fcb4..1f6fce0 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -13,7 +13,7 @@ SYNOPSIS

 DESCRIPTION
 ---
-This script is used to move or rename a file, directory or symlink.
+Move or rename a file, directory or symlink.

  git mv [-v] [-f] [-n] [-k] source destination
  git mv [-v] [-f] [-n] [-k] source ... destination directory
@@ -44,6 +44,12 @@ OPTIONS
 --verbose::
Report the names of files as they are moved.

+SUBMODULES
+--
+Moving a submodule using a gitfile (which means they were cloned
+with a Git version 1.7.8 or newer) will update the gitfile and
+core.worktree setting to make the submodule work in the new location.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/mv.c b/builtin/mv.c
index 1d3ef63..68b7060 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -9,6 +9,7 @@
 #include cache-tree.h
 #include string-list.h
 #include parse-options.h
+#include submodule.h

 static const char * const builtin_mv_usage[] = {
N_(git mv [options] source... destination),
@@ -66,7 +67,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN('k', NULL, ignore_errors, N_(skip move/rename 
errors)),
OPT_END(),
};
-   const char **source, **destination, **dest_path;
+   const char **source, **destination, **dest_path, **submodule_gitfile;
enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -85,6 +86,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
source = internal_copy_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
dest_path = internal_copy_pathspec(prefix, argv + argc, 1, 0);
+   submodule_gitfile = xcalloc(argc, sizeof(char *));

if (dest_path[0][0] == '\0')
/* special case: . was normalized to  */
@@ -120,8 +122,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
else if (src_is_dir) {
int first = cache_name_pos(src, length);
if (first = 0) {
+   struct strbuf submodule_dotgit = STRBUF_INIT;
if (!S_ISGITLINK(active_cache[first]-ce_mode))
die (_(Huh? Directory %s is in index 
and no submodule?), src);
+   strbuf_addf(submodule_dotgit, %s/.git, src);
+   submodule_gitfile[i] = 
read_gitfile(submodule_dotgit.buf);
+   if (submodule_gitfile[i])
+   submodule_gitfile[i] = 
xstrdup(submodule_gitfile[i]);
+   strbuf_release(submodule_dotgit);
} else {
const char *src_w_slash = add_slash(src);
int last, len_w_slash = length + 1;
@@ -216,9 +224,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
int pos;
if (show_only || verbose)
printf(_(Renaming %s to %s\n), src, dst);
-   if (!show_only  mode != INDEX 
-   rename(src, dst)  0  !ignore_errors)
-   die_errno (_(renaming '%s' failed), src);
+   if (!show_only  mode != INDEX) {
+   if (rename(src, dst)  0  !ignore_errors)
+   die_errno (_(renaming '%s' failed), src);
+   if (submodule_gitfile[i])
+   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
+   }

if (mode == WORKING_DIRECTORY)

[PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions

Add the new is_staging_gitmodules_ok() and stage_updated_gitmodules()
functions to submodule.c. The first makes it possible for call sites to
see if the .gitmodules file did contain any unstaged modifications they
would accidentally stage in addition to those they intend to stage
themselves. The second function stages all modifications to the
.gitmodules file, both will be used by subsequent patches for the mv
and rm commands.

Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
 submodule.c | 51 +++
 submodule.h |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/submodule.c b/submodule.c
index d96d187..584f7de 100644
--- a/submodule.c
+++ b/submodule.c
@@ -10,6 +10,7 @@
 #include string-list.h
 #include sha1-array.h
 #include argv-array.h
+#include blob.h

 static struct string_list config_name_for_path;
 static struct string_list config_fetch_recurse_submodules_for_name;
@@ -30,6 +31,51 @@ static struct sha1_array ref_tips_after_fetch;
  */
 static int gitmodules_is_unmerged;

+/*
+ * This flag is set if the .gitmodules file had unstaged modifications on
+ * startup. This must be checked before allowing modifications to the
+ * .gitmodules file with the intention to stage them later, because when
+ * continuing we would stage the modifications the user didn't stage herself
+ * too. That might change in a future version when we learn to stage the
+ * changes we do ourselves without staging any previous modifications.
+ */
+static int gitmodules_is_modified;
+
+
+int is_staging_gitmodules_ok()
+{
+   return !gitmodules_is_modified;
+}
+
+void stage_updated_gitmodules(void)
+{
+   struct strbuf buf = STRBUF_INIT;
+   struct stat st;
+   int pos;
+   struct cache_entry *ce;
+   int namelen = strlen(.gitmodules);
+
+   pos = cache_name_pos(.gitmodules, namelen);
+   if (pos  0) {
+   warning(_(could not find .gitmodules in index));
+   return;
+   }
+   ce = active_cache[pos];
+   ce-ce_flags = namelen;
+   if (strbuf_read_file(buf, .gitmodules, 0)  0)
+   die(_(reading updated .gitmodules failed));
+   if (lstat(.gitmodules, st)  0)
+   die_errno(_(unable to stat updated .gitmodules));
+   fill_stat_cache_info(ce, st);
+   ce-ce_mode = ce_mode_from_stat(ce, st.st_mode);
+   if (remove_cache_entry_at(pos)  0)
+   die(_(unable to remove .gitmodules from index));
+   if (write_sha1_file(buf.buf, buf.len, blob_type, ce-sha1))
+   die(_(adding updated .gitmodules failed));
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+   die(_(staging updated .gitmodules failed));
+}
+
 static int add_submodule_odb(const char *path)
 {
struct strbuf objects_directory = STRBUF_INIT;
@@ -116,6 +162,11 @@ void gitmodules_config(void)
!memcmp(ce-name, .gitmodules, 11))
gitmodules_is_unmerged = 1;
}
+   } else if (pos  active_nr) {
+   struct stat st;
+   if (lstat(.gitmodules, st) == 0 
+   ce_match_stat(active_cache[pos], st, 0)  
DATA_CHANGED)
+   gitmodules_is_modified = 1;
}

if (!gitmodules_is_unmerged)
diff --git a/submodule.h b/submodule.h
index 29e9658..244d5f5 100644
--- a/submodule.h
+++ b/submodule.h
@@ -11,6 +11,8 @@ enum {
RECURSE_SUBMODULES_ON = 2
 };

+int is_staging_gitmodules_ok();
+void stage_updated_gitmodules(void);
 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);
-- 
1.8.4.rc0.199.g7079aac


--
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 v3 4/5] Teach mv to update the path entry in .gitmodules for moved submodules

Currently using git mv on a submodule moves the submodule's work tree in
that of the superproject. But the submodule's path setting in .gitmodules
is left untouched, which is now inconsistent with the work tree and makes
git commands that rely on the proper path - name mapping (like status and
diff) behave strangely.

Let git mv help here by not only moving the submodule's work tree but
also updating the submodule.submodule name.path setting from the
.gitmodules file and stage both. This doesn't happen when no .gitmodules
file is found and only issues a warning when it doesn't have a section for
this submodule. This is because the user might just use plain gitlinks
without the .gitmodules file or has already updated the path setting by
hand before issuing the git mv command (in which case the warning
reminds him that mv would have done that for him). Only when .gitmodules
is found and contains merge conflicts the mv command will fail and tell
the user to resolve the conflict before trying again.

Also extend the man page to inform the user about this new feature.

Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
 Documentation/git-mv.txt |  2 ++
 builtin/mv.c | 10 ++-
 submodule.c  | 33 +
 submodule.h  |  1 +
 t/t7001-mv.sh| 75 
 5 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 1f6fce0..b1f7988 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -49,6 +49,8 @@ SUBMODULES
 Moving a submodule using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will update the gitfile and
 core.worktree setting to make the submodule work in the new location.
+It also will attempt to update the submodule.name.path setting in
+the linkgit:gitmodules[5] file and stage that file (unless -n is used).

 GIT
 ---
diff --git a/builtin/mv.c b/builtin/mv.c
index 68b7060..7dd6bb4 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -58,7 +58,7 @@ static struct lock_file lock_file;

 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
-   int i, newfd;
+   int i, newfd, gitmodules_modified = 0;
int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
struct option builtin_mv_options[] = {
OPT__VERBOSE(verbose, N_(be verbose)),
@@ -72,6 +72,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;

+   gitmodules_config();
git_config(git_default_config, NULL);

argc = parse_options(argc, argv, prefix, builtin_mv_options,
@@ -125,6 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
struct strbuf submodule_dotgit = STRBUF_INIT;
if (!S_ISGITLINK(active_cache[first]-ce_mode))
die (_(Huh? Directory %s is in index 
and no submodule?), src);
+   if (!is_staging_gitmodules_ok())
+   die (_(Please, stage your changes to 
.gitmodules or stash them to proceed));
strbuf_addf(submodule_dotgit, %s/.git, src);
submodule_gitfile[i] = 
read_gitfile(submodule_dotgit.buf);
if (submodule_gitfile[i])
@@ -229,6 +232,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
die_errno (_(renaming '%s' failed), src);
if (submodule_gitfile[i])
connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
+   if (!update_path_in_gitmodules(src, dst))
+   gitmodules_modified = 1;
}

if (mode == WORKING_DIRECTORY)
@@ -240,6 +245,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
rename_cache_entry_at(pos, dst);
}

+   if (gitmodules_modified)
+   stage_updated_gitmodules();
+
if (active_cache_changed) {
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
diff --git a/submodule.c b/submodule.c
index 584f7de..b210685 100644
--- a/submodule.c
+++ b/submodule.c
@@ -47,6 +47,39 @@ int is_staging_gitmodules_ok()
return !gitmodules_is_modified;
 }

+/*
+ * Try to update the path entry in the submodule.name section of the
+ * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=oldpath setting was found and we could update it.
+ */
+int update_path_in_gitmodules(const char *oldpath, const char *newpath)
+{
+   struct strbuf entry = STRBUF_INIT;
+   struct string_list_item *path_option;
+
+   if (!file_exists(.gitmodules))

[PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

Currently using git rm on a submodule removes the submodule's work tree
from that of the superproject and the gitlink from the index. But the
submodule's section in .gitmodules is left untouched, which is a leftover
of the now removed submodule and might irritate users (as opposed to the
setting in .git/config, this must stay as a reminder that the user showed
interest in this submodule so it will be repopulated later when an older
commit is checked out).

Let git rm help the user by not only removing the submodule from the
work tree but by also removing the submodule.submodule name section
from the .gitmodules file and stage both. This doesn't happen when the
--cached option is used, as it would modify the work tree. This also
silently does nothing when no .gitmodules file is found and only issues a
warning when it doesn't have a section for this submodule. This is because
the user might just use plain gitlinks without the .gitmodules file or has
already removed the section by hand before issuing the git rm command
(in which case the warning reminds him that rm would have done that for
him). Only when .gitmodules is found and contains merge conflicts the rm
command will fail and tell the user to resolve the conflict before trying
again.

Also extend the man page to inform the user about this new feature. While
at it promote the submodule sub-section to a chapter as it made not much
sense under REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM.

In t7610 three uses of git rm submod had to be replaced with git rm
--cached submod because that test expects .gitmodules and the work tree
to stay untouched. Also in t7400 the tests for the remaining settings in
the .gitmodules file had to be changed to assert that these settings are
missing.

Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
 Documentation/git-rm.txt   |  8 ++--
 builtin/rm.c   | 19 +++--
 submodule.c| 32 +++
 submodule.h|  1 +
 t/t3600-rm.sh  | 98 +++---
 t/t7400-submodule-basic.sh | 14 ++-
 t/t7610-mergetool.sh   |  6 +--
 7 files changed, 153 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 1d876c2..9d731b4 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -134,14 +134,16 @@ use the following command:
 git diff --name-only --diff-filter=D -z | xargs -0 git rm --cached
 

-Submodules
-~~
+SUBMODULES
+--
 Only submodules using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will be removed from the work
 tree, as their repository lives inside the .git directory of the
 superproject. If a submodule (or one of those nested inside it)
 still uses a .git directory, `git rm` will fail - no matter if forced
-or not - to protect the submodule's history.
+or not - to protect the submodule's history. If it exists the
+submodule.name section in the linkgit:gitmodules[5] file will also
+be removed and that file will be staged (unless --cached or -n are used).

 A submodule is considered up-to-date when the HEAD is the same as
 recorded in the index, no tracked files are modified and no untracked
diff --git a/builtin/rm.c b/builtin/rm.c
index df85f98..a9d45dd 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -283,6 +283,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
struct pathspec pathspec;
char *seen;

+   gitmodules_config();
git_config(git_default_config, NULL);

argc = parse_options(argc, argv, prefix, builtin_rm_options,
@@ -324,7 +325,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
continue;
ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
list.entry[list.nr].name = ce-name;
-   list.entry[list.nr++].is_submodule = S_ISGITLINK(ce-ce_mode);
+   list.entry[list.nr].is_submodule = S_ISGITLINK(ce-ce_mode);
+   if (list.entry[list.nr++].is_submodule 
+   !is_staging_gitmodules_ok())
+   die (_(Please, stage your changes to .gitmodules or 
stash them to proceed));
}

if (pathspec.nr) {
@@ -396,13 +400,15 @@ int cmd_rm(int argc, const char **argv, const char 
*prefix)
 * in the middle)
 */
if (!index_only) {
-   int removed = 0;
+   int removed = 0, gitmodules_modified = 0;
for (i = 0; i  list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
if (is_empty_dir(path)) {
if (!rmdir(path)) {
removed = 1;
+   if 
(!remove_path_from_gitmodules(path))
+

Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure

2013-07-30 Thread Jeff King

On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:

 Brandon Casey bca...@nvidia.com writes:
 
  From: Brandon Casey draf...@gmail.com
 
  When the number of open packs exceeds pack_max_fds, unuse_one_window()
  is called repeatedly to attempt to release the least-recently-used
  pack windows, which, as a side-effect, will also close a pack file
  after closing its last open window.  If a pack file has been opened,
  but no windows have been allocated into it, it will never be selected
  by unuse_one_window() and hence its file descriptor will not be
  closed.  When this happens, git may exceed the number of file
  descriptors permitted by the system.
 
 An interesting find.  The patch from a cursory look reads OK.

Yeah. I wonder if unuse_one_window() should actually leave the pack fd
open now in general.

If you close packfile descriptors, you can run into racy situations
where somebody else is repacking and deleting packs, and they go away
while you are trying to access them. If you keep a descriptor open,
you're fine; they last to the end of the process. If you don't, then
they disappear from under you.

For normal object access, this isn't that big a deal; we just rescan the
packs and retry. But if you are packing yourself (e.g., because you are
a pack-objects started by upload-pack for a clone or fetch), it's much
harder to recover (and we print some warnings).

We had our core.packedGitWindowSize lowered on GitHub for a while, and
we ran into this warning on busy repositories when we were running git
gc on the server. We solved it by bumping the window size so we never
release memory.

But just not closing the descriptor wouldn't work until Brandon's patch,
because we used the same function to release memory and descriptor
pressure. Now we could do them separately (and progressively if we need
to).

  This is not likely to occur during upload-pack since upload-pack
  reads each object from the pack so that it can peel tags and
  advertise the exposed object.
 
 Another interesting find.  Perhaps there is a room for improvements,
 as packed-refs file knows what objects the tags peel to?  I vaguely
 recall Peff was actively reducing the object access during ref
 enumeration in not so distant past...

Yeah, we should be reading almost no objects these days due to the
packed-refs peel lines. I just did a double-check on what git
upload-pack . /dev/null /dev/null reads on my git.git repo, and it is
only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.
In other words, the tags I got since the last time I ran git gc. So I
think all is working as designed.

We could give receive-pack the same treatment; I've spent less time
micro-optimizing it because because we (and most sites, I would think)
get an order of magnitude more fetches than pushes.

-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 v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

2013-07-30 Thread Fredrik Gustafsson

On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:
 +/*
 + * Try to remove the submodule.name section from .gitmodules where the 
 given
 + * path is configured. Return 0 only if a .gitmodules file was found, a 
 section
 + * with the correct path=path setting was found and we could remove it.
 + */
 +int remove_path_from_gitmodules(const char *path)
 +{
 + struct strbuf sect = STRBUF_INIT;
 + struct string_list_item *path_option;
 +
 + if (!file_exists(.gitmodules)) /* Do nothing without .gitmodules */
 + return -1;
 +
 + if (gitmodules_is_unmerged)
 + die(_(Cannot change unmerged .gitmodules, resolve merge 
 conflicts first));
 +
 + path_option = unsorted_string_list_lookup(config_name_for_path, path);
 + if (!path_option) {
 + warning(_(Could not find section in .gitmodules where 
 path=%s), path);
 + return -1;
 + }
 + strbuf_addstr(sect, submodule.);
 + strbuf_addstr(sect, path_option-util);
 + if (git_config_rename_section_in_file(.gitmodules, sect.buf, NULL)  
 0) {
 + /* Maybe the user already did that, don't error out here */
 + warning(_(Could not remove .gitmodules entry for %s), path);
 + return -1;
 + }
 + strbuf_release(sect);
 + return 0;
 +}

This question applies for this function and a few more functions in this
patch that has the same characteristics.

If we're in a state when we need to return non-zero, we don't do any
cleaning (that is strbuf_release()). Since this file is in the part
called libgit AFAIK, shouldn't we always clean after us?

Would it make sense to have different return values for different
errors?

I do like the comments above the function, more functions (at least
non-static ones) should follow this good style IMHO.
-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
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 5/3] revert most of the http_options() change

Kyle J. McKay mack...@gmail.com writes:

 And now all the t5200-url-normalize tests pass again.

 FYI, I couldn't get the patches to apply against next or pu without
 some minor tweaks that were just conflict resolutions having to do
 with git_config_with_options changing its signature.

Thanks.

I built these five patches directly on top of your series, i.e. on
top of cea9928d (docs: update http.url.* options documentation,
2013-07-25).

I'll rebuild the series with your fixes and I may even queue it to
'pu', but with some random thoughts:

 * url-match.[ch] are ugly names.  I'd be happier with
   urlmatch.[ch].

 * In the end result, http_options() looks mostly identical to that
   of the 'master', but it got an extra void *matched, only to
   support git config --get-urlmatch.  I however do not have to,
   if I do a few things:

   - Instead of implementing urlmatch_config_item that extends
 urlmatch_item, have a separate string_list, keyed by the
 variable names, and point the string_list with the generic cb
 pointer I already have in urlmatch_config.  The fn() on the
 git config side has to index this second string_list with the
 variable and keep track of the value from the best candidate we
 have seen so far.

   - The above allows us to lose item_alloc and item_clear callback
 functions from urlmatch_config, as we will not be doing the
 structure inheritance to extend urlmatch_item.

   - We still need cascade_fn callback, though.

 * With the above, http_options() does not have to change in the
   series.  We could restructure the series perhaps this way:

   - http.sslCertPasswordProtected parsing fix.

   - Add urlmatch.[ch], which would be your config: improve support
 for http.url.* settings and yesterday's url-match: split
 out URL matching logic out of http.c, and a half of
 url-match: generalize configuration collection logic.

   - Update http.c to use urlmatch.[ch] to make http.c match the
 result of the endgame patch [5/3], and add necessary end user
 documentation to Documentation/config.txt.

   - Add test-url-normalize and t/t5200

   - Update builtin/config.c for --get-urlmatch, to make
 builtin/config.c match the result of the endgame patch [5/3],
 add end user documentation to Documentation/git-config.txt.

   - Add some tests for git config --get-urlmatch.

--
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] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

2013-07-30 Thread Stefan Beller

On 07/30/13 21:24, Junio C Hamano wrote:
 Stefan Beller stefanbel...@googlemail.com writes:
 
 As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),
 the OPT_BOOLEAN was deprecated.
 While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or
 the OPT_COUNTUP to keep existing behavior, this commit is actually a
 bug fix!

 In line 499 we have:
  if (list + delete + verify  1)
  usage_with_options(git_tag_usage, options);
 Now if we give one of the options twice, we'll get the usage information.
 (i.e. 'git tag --verify --verify tagname' and
 'git --delete --delete tagname' yield usage information and do not
 do the intended command.)

 This could have been fixed by rewriting the line to
  if (!!list + !!delete + !!verify  1)
  usage_with_options(git_tag_usage, options);
 or as it happened in this patch by having the parameters not
 counting up for each occurrence, but the OPT_BOOL just setting the
 variables to either 0 if the option is not given or 1 if the option is
 given multiple times.
 
 Makes twisted sort of sense ;-).
 
 However we could discuss if the negated options do make sense
 here, or if we don't want to allow them here, as this seems valid
 (before and after this patch):

  git tag --no-verify --delete tagname
 
 It probably does not.  As you hinted in your earlier patch, we may
 want to introduce a only can set to true boolean used solely to
 specify these things.  They are disguised as options, but are in
 fact command operation modes that are often mutually exclusive.
 
 For these operation modes that are mutually exclusive, there are
 multiple possible implementations:
 
  * One OPT_BOOL_NONEG per option; the code ensures the mutual
exclusion with if (list + delete + verify  1);
 
  * One OPT_BIT per option in a single variable; the code ensures the
mutual exclusion with count_bits, which may be a lot more
cumbersome;
 
  * OPT_SET_INT that updates a single variable to enum; instead of
making it an error to give two conflicting modes, this would give
us the last-one-wins rule.
 
 Unlike usual options, we generally do not want the last-one-wins
 semantics for command operation modes, I think.
 
 Perhaps we would want something like this?
 
 -- 8 --
 Subject: [PATCH] parse-options: add OPT_CMDMODE()
 
 This can be used to define a set of mutually exclusive command
 mode options, and automatically catch use of more than one from
 that set as an error.
 
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  parse-options.c | 58 
 -
  parse-options.h |  3 +++
  2 files changed, 56 insertions(+), 5 deletions(-)
 
 diff --git a/parse-options.c b/parse-options.c
 index c2cbca2..62e9b1c 100644
 --- a/parse-options.c
 +++ b/parse-options.c
 @@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char 
 **file)
   *file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));
  }
  
 +static int opt_command_mode_error(const struct option *opt,
 +   const struct option *all_opts,
 +   int flags)
 +{
 + const struct option *that;
 + struct strbuf message = STRBUF_INIT;
 + struct strbuf that_name = STRBUF_INIT;
 +
 + /*
 +  * Find the other option that was used to set the variable
 +  * already, and report that this is not compatible with it.
 +  */
 + for (that = all_opts; that-type != OPTION_END; that++) {
 + if (that == opt ||
 + that-type != OPTION_CMDMODE ||
 + that-value != opt-value ||
 + that-defval != *(int *)opt-value)
 + continue;
 +
 + if (that-long_name)
 + strbuf_addf(that_name, --%s, that-long_name);
 + else
 + strbuf_addf(that_name, -%c, that-short_name);
 + strbuf_addf(message, : incompatible with %s, that_name.buf);
 + strbuf_release(that_name);
 + opterror(opt, message.buf, flags);
 + strbuf_release(message);
 + return -1;
 + }
 + return opterror(opt, : incompatible with something else, flags);
 +}
 +
  static int get_value(struct parse_opt_ctx_t *p,
 -  const struct option *opt, int flags)
 +  const struct option *opt,
 +  const struct option *all_opts,
 +  int flags)
  {
   const char *s, *arg;
   const int unset = flags  OPT_UNSET;
 @@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p,
   *(int *)opt-value = unset ? 0 : opt-defval;
   return 0;
  
 + case OPTION_CMDMODE:
 + /*
 +  * Giving the same mode option twice, although is unnecessary,
 +  * is not a grave error, so let it pass.
 +  */
 + if (*(int *)opt-value  *(int *)opt-value != opt-defval)
 +

Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

2013-07-30 Thread Stefan Beller

On 07/30/13 21:24, Junio C Hamano wrote:
 
 ... and then git tag may become like so.
 
  builtin/tag.c | 27 ---
  1 file changed, 12 insertions(+), 15 deletions(-)
 
 diff --git a/builtin/tag.c b/builtin/tag.c
 index af3af3f..d8ae5aa 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -436,18 +436,18 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   struct ref_lock *lock;
   struct create_tag_options opt;
   char *cleanup_arg = NULL;
 - int annotate = 0, force = 0, lines = -1, list = 0,
 - delete = 0, verify = 0;
 + int annotate = 0, force = 0, lines = -1;
 + int cmdmode = 0;
   const char *msgfile = NULL, *keyid = NULL;
   struct msg_arg msg = { 0, STRBUF_INIT };
   struct commit_list *with_commit = NULL;
   struct option options[] = {
 - OPT_BOOLEAN('l', list, list, N_(list tag names)),
 + OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
   { OPTION_INTEGER, 'n', NULL, lines, N_(n),
   N_(print n lines of each tag message),
   PARSE_OPT_OPTARG, NULL, 1 },
 - OPT_BOOLEAN('d', delete, delete, N_(delete tags)),
 - OPT_BOOLEAN('v', verify, verify, N_(verify tags)),
 + OPT_CMDMODE('d', delete, cmdmode, N_(delete tags), 'd'),
 + OPT_CMDMODE('v', verify, cmdmode, N_(verify tags), 'v'),
  
   OPT_GROUP(N_(Tag creation options)),
   OPT_BOOLEAN('a', annotate, annotate,
 @@ -489,22 +489,19 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   }
   if (opt.sign)
   annotate = 1;
 - if (argc == 0  !(delete || verify))
 - list = 1;
 + if (argc == 0  !cmdmode)
 + cmdmode = 'l';
  
 - if ((annotate || msg.given || msgfile || force) 
 - (list || delete || verify))
 + if ((annotate || msg.given || msgfile || force)  (cmdmode != 0))
   usage_with_options(git_tag_usage, options);
  
 - if (list + delete + verify  1)
 - usage_with_options(git_tag_usage, options);
   finalize_colopts(colopts, -1);
 - if (list  lines != -1) {
 + if (cmdmode == 'l'  lines != -1) {
   if (explicitly_enable_column(colopts))
   die(_(--column and -n are incompatible));
   colopts = 0;
   }
 - if (list) {
 + if (cmdmode == 'l') {
   int ret;
   if (column_active(colopts)) {
   struct column_options copts;
 @@ -523,9 +520,9 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   die(_(--contains option is only allowed with -l.));
   if (points_at.nr)
   die(_(--points-at option is only allowed with -l.));
 - if (delete)
 + if (cmdmode == 'd')
   return for_each_tag_name(argv, delete_tag);
 - if (verify)
 + if (cmdmode == 'v')
   return for_each_tag_name(argv, verify_tag);
  
   if (msg.given || msgfile) {


Here is just another idea: 
if (cmdmode == 'v')
This may be hard to read, (What is 'v'? I cannot remember 
all the alphabet ;)) So maybe we could have an enum instead of
the last parameter? 
OPT_CMDMODE( short, long, variable, description, enum)

Also the variable would then only need to be an enum accepting variable,
and not an integer accepting all integer range, so we'd also catch 
typos or wrong values in such a case:
 + if (argc == 0  !cmdmode)
 + cmdmode = 'l'; // maybe 'l' is a typo and not existing?



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions

Jens Lehmann jens.lehm...@web.de writes:

 +int is_staging_gitmodules_ok()

Will tweak this to:

int is_staging_gitmodules_ok(void)

and fix this as well:

 +int is_staging_gitmodules_ok();
 +void stage_updated_gitmodules(void);


before queuing.

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: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

Stefan Beller stefanbel...@googlemail.com writes:

 Your approach seems more like what we really want, however I'd have
 some points:
  * Is it a good idea to have so many different OPT_MODE or
OPTION_MODE defines? In my attempts I tried to reuse existing
OPTION_s to not pollute the parsing infrastructure with more
lines of code. ;)

  * You can only have one OPTION_CMDMODE in one argv vector right?

That is not what I intended, at least.

int one = 0, two = 0;
struct option options[] = {
OPT_CMDMODE('a', NULL, one, N_(set one to a), 'a'),
OPT_CMDMODE('b', NULL, one, N_(set one to b), 'b'),
OPT_CMDMODE('c', NULL, two, N_(set two to c), 'c'),
OPT_CMDMODE('d', NULL, two, N_(set two to d), 'd'),
OPT_END()
}

should give you two independent sets of modes, one and two.

The only reason I needed to add an extra parameter to get_value()
was so that I can tell the former two and the latter two belong to
different groups, and that is done by looking at the address of the
variable.  In opt_command_mode_error(), opt-value == that-value
is used as a condition to see if the other option is possibly the
one that was used previously, which conflicted with us.

--
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] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

Stefan Beller stefanbel...@googlemail.com writes:

 Here is just another idea: 
   if (cmdmode == 'v')
 This may be hard to read, (What is 'v'? I cannot remember 
 all the alphabet ;)) So maybe we could have an enum instead of
 the last parameter? 
 OPT_CMDMODE( short, long, variable, description, enum)

I actually was thinking about going totally in the opposite
direction.

People who grew up with the old Unix tradition getopt(3) are used to
code like this:

while ((opt = getopt(ac, av, abcde)) != -1) {
switch (opt) {
case 'a': perform_a(); break;
case 'b': perform_b(); break;
...
}
}

In other words, the enum is most convenient if it matches the
short option, so instead of having to repeat ourselves over and
over, like I did in that illustration patch for builtin/tag.c, e.g.

OPT_CMDMODE('d', delete, cmdmode, N_(delete tags), 'd'),
OPT_CMDMODE('v', verify, cmdmode, N_(verify tags), 'v'),

we could just do

#define OPT_CMDMODE(s, l, v, h) \
{ OPTION_CMDMODE, (s), (l), (v), NULL, \
  (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }

--
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] sha1_file: introduce close_one_pack() to close packs on fd pressure

2013-07-30 Thread Brandon Casey

On Tue, Jul 30, 2013 at 12:52 PM, Jeff King p...@peff.net wrote:
 On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:

 Brandon Casey bca...@nvidia.com writes:

  From: Brandon Casey draf...@gmail.com
 
  When the number of open packs exceeds pack_max_fds, unuse_one_window()
  is called repeatedly to attempt to release the least-recently-used
  pack windows, which, as a side-effect, will also close a pack file
  after closing its last open window.  If a pack file has been opened,
  but no windows have been allocated into it, it will never be selected
  by unuse_one_window() and hence its file descriptor will not be
  closed.  When this happens, git may exceed the number of file
  descriptors permitted by the system.

 An interesting find.  The patch from a cursory look reads OK.

 Yeah. I wonder if unuse_one_window() should actually leave the pack fd
 open now in general.

 If you close packfile descriptors, you can run into racy situations
 where somebody else is repacking and deleting packs, and they go away
 while you are trying to access them. If you keep a descriptor open,
 you're fine; they last to the end of the process. If you don't, then
 they disappear from under you.

 For normal object access, this isn't that big a deal; we just rescan the
 packs and retry. But if you are packing yourself (e.g., because you are
 a pack-objects started by upload-pack for a clone or fetch), it's much
 harder to recover (and we print some warnings).

 We had our core.packedGitWindowSize lowered on GitHub for a while, and
 we ran into this warning on busy repositories when we were running git
 gc on the server. We solved it by bumping the window size so we never
 release memory.

 But just not closing the descriptor wouldn't work until Brandon's patch,
 because we used the same function to release memory and descriptor
 pressure. Now we could do them separately (and progressively if we need
 to).

I had thought about whether to stop closing the pack file in
unuse_one_window(), but didn't have a reason to do so.  I think the
scenario you described provides a justification.  If we're not under
file descriptor pressure and we can possibly avoid rescanning the pack
directory, it sounds like a net win.

  This is not likely to occur during upload-pack since upload-pack
  reads each object from the pack so that it can peel tags and
  advertise the exposed object.

 Another interesting find.  Perhaps there is a room for improvements,
 as packed-refs file knows what objects the tags peel to?  I vaguely
 recall Peff was actively reducing the object access during ref
 enumeration in not so distant past...

 Yeah, we should be reading almost no objects these days due to the
 packed-refs peel lines. I just did a double-check on what git
 upload-pack . /dev/null /dev/null reads on my git.git repo, and it is
 only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.
 In other words, the tags I got since the last time I ran git gc. So I
 think all is working as designed.

Ok, looks like this has been the case since your 435c8332 which taught
upload-pack to use peel_ref().  So looks like we do avoid reaching
into the pack for any ref that was read from a (modern) packed-refs
file.  The repository I was testing with had mostly loose refs.
Indeed, after packing refs, upload-pack encounters the same problem as
receive-pack and runs out of file descriptors.

So my comment about upload-pack is not completely accurate.
Upload-pack _can_ run into this problem, but the refs must be packed,
as well as there being enough of them that exist in enough different
pack files to exceed the processes fd limit.

 We could give receive-pack the same treatment; I've spent less time
 micro-optimizing it because because we (and most sites, I would think)
 get an order of magnitude more fetches than pushes.

I don't think it would need the 435c8332 treatment since receive-pack
doesn't peel refs when it advertises them to the client and hence does
not need to load the ref object from the pack file during ref
advertisement, but possibly some of the other stuff you did would be
applicable.  But like you said, the number of fetches far exceed the
number of pushes.

-Brandon
--
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 v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

Am 30.07.2013 22:15, schrieb Fredrik Gustafsson:
 On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:
 +/*
 + * Try to remove the submodule.name section from .gitmodules where the 
 given
 + * path is configured. Return 0 only if a .gitmodules file was found, a 
 section
 + * with the correct path=path setting was found and we could remove it.
 + */
 +int remove_path_from_gitmodules(const char *path)
 +{
 +struct strbuf sect = STRBUF_INIT;
 +struct string_list_item *path_option;
 +
 +if (!file_exists(.gitmodules)) /* Do nothing without .gitmodules */
 +return -1;
 +
 +if (gitmodules_is_unmerged)
 +die(_(Cannot change unmerged .gitmodules, resolve merge 
 conflicts first));
 +
 +path_option = unsorted_string_list_lookup(config_name_for_path, path);
 +if (!path_option) {
 +warning(_(Could not find section in .gitmodules where 
 path=%s), path);
 +return -1;
 +}
 +strbuf_addstr(sect, submodule.);
 +strbuf_addstr(sect, path_option-util);
 +if (git_config_rename_section_in_file(.gitmodules, sect.buf, NULL)  
 0) {
 +/* Maybe the user already did that, don't error out here */
 +warning(_(Could not remove .gitmodules entry for %s), path);
 +return -1;
 +}
 +strbuf_release(sect);
 +return 0;
 +}
 
 This question applies for this function and a few more functions in this
 patch that has the same characteristics.
 
 If we're in a state when we need to return non-zero, we don't do any
 cleaning (that is strbuf_release()). Since this file is in the part
 called libgit AFAIK, shouldn't we always clean after us?

Right you are, thanks for bringing that up. The last return needs a
strbuf_release(), will fix that in v4.

 Would it make sense to have different return values for different
 errors?

I don't think so. The caller only wants to know if the modification
of the .gitmodules file happened or not, so he can decide if that
needs to be staged.
--
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 v3 3/5] submodule.c: add .gitmodules staging helper functions

Am 30.07.2013 23:37, schrieb Junio C Hamano:
 Jens Lehmann jens.lehm...@web.de writes:
 
 +int is_staging_gitmodules_ok()
 
 Will tweak this to:
 
   int is_staging_gitmodules_ok(void)
 
 and fix this as well:
 
 +int is_staging_gitmodules_ok();
 +void stage_updated_gitmodules(void);
 
 
 before queuing.

Thanks. As Frederik already noticed, this series is missing at least
one strbuf_release(). Will fix both in v4.
--
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 0/3] Remove sq_quote_print() in favor of *_buf