Re: [PATCH 0/4] remote-hg: more improvements

[David Kastrup]

Felipe Contreras felipe.contre...@gmail.com writes:

 David Kastrup wrote:
 Shouting your God is an imaginary shithead every time you see Mark

 There's no point in discussing with an unconstructive person as you.

So go to a constructive person you call your friend and show him one of
your calm rational mails and ask him for his opinion.  Did you do so?
Why not?  Are you so infallible in communication that a second opinion
from someone you trust would be irrelevant?  Or is there nobody you
trust?

 I've made my point, you are just talking nonsense.

 Every rational person on this list knows that if I wanted to, I could
 be much more offsensive and insulting.

It would not appear that there any rational persons on this list apart
from you.  Not that it matters.  I think that all the other irrational
people on this list are rather more interested in your potential of
being less offensive and insulting.  Once the cure is worse than the
ailment, namely you cause fewer work to be done on Git by your presence
than you can do yourself, because of you distracting others and/or
making them leave, it does not matter whether it could be even worse.

-- 
David Kastrup
--
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] transport-helper: add trailing --

[Felipe Contreras]

Stepan Kasal wrote:
 From: Sverre Rabbelier srabbel...@gmail.com
 Date: Sat, 28 Aug 2010 20:49:01 -0500
 
 [PT: ensure we add an additional element to the argv array]
 
 Signed-off-by: Stepan Kasal ka...@ucw.cz
 ---
 
 Hi,
   this patch was present in msysgit from Mar 2012.
 Do you like it?

Adding an extra '--' would make sense if we do something like
'git fast-export $stuff master' and there was a file named master.
However, we do 'git fast-export $stuff refs/heads/master', which case
the '--' is a no-nop *always*.

It doesn't hurt to add it though. I think.

-- 
Felipe Contreras
--
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/RFC] Always auto-gc after calling a fast-import transport

[Felipe Contreras]

Stepan Kasal wrote:
 From: Johannes Schindelin johannes.schinde...@gmx.de
 Date: Mon, 9 Apr 2012 13:04:35 -0500
 
 After importing anything with fast-import, we should always let the
 garbage collector do its job, since the objects are written to disk
 inefficiently.

I'm not sure how slow `git gc --auto` is with a totally unpacked
repository, but it guess it will be slow enough that some people might
not want to do the gc initially.

Not many people have complained about the size of the repoitory after
cloning, so I think it should be OK to let users call `git gc` when they
wish to.

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

[no subject]

[Christian Organization]

We are Christian organization, we give loan or financial help to those in
need, contact us at marieloanlend...@gmail.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: [GUILT v2 28/29] Added guilt.reusebranch configuration option.

[Per Cederqvist]

On Wed, May 14, 2014 at 5:53 PM, Jeff Sipek jef...@josefsipek.net wrote:
 On Tue, May 13, 2014 at 10:31:04PM +0200, Per Cederqvist wrote:
 When the option is true (the default), Guilt does not create a new Git
 branch when patches are applied.  This way, you can switch between
 Guilt 0.35 and the current version of Guilt with no issues.

 At a future time, maybe a year after Guilt with guilt.reusebranch
 support is released, the default should be changed to false to take
 advantage of the ability to use a separate Git branch when patches are
 applied.

 Signed-off-by: Per Cederqvist ced...@opera.com
 ---
  guilt|  28 +++-
  regression/scaffold  |   1 +
  regression/t-062.out | 441 
 +++
  regression/t-062.sh  | 137 
  4 files changed, 601 insertions(+), 6 deletions(-)
  create mode 100644 regression/t-062.out
  create mode 100755 regression/t-062.sh
 ...
 diff --git a/guilt b/guilt
 index 9947acc..7c830eb 100755
 --- a/guilt
 +++ b/guilt
 ...
 @@ -928,13 +935,22 @@ else
   die Unsupported operating system: $UNAME_S
  fi

 -if [ $branch = $raw_git_branch ]  [ -n `get_top 2/dev/null` ]
 -then
 -# This is for compat with old repositories that still have a
 -# pushed patch without the new-style branch prefix.
 -old_style_prefix=true
 +if [ -n `get_top 2/dev/null` ]; then
 + # If there is at least one pushed patch, we set
 + # old_style_prefix according to how it was pushed.  It is only
 + # possible to change the prefix style while no patches are
 + # applied.
 + if [ $branch = $raw_git_branch ]; then
 + old_style_prefix=true
 + else
 + old_style_prefix=false
 + fi
  else
 -old_style_prefix=false
 + if $reuse_branch; then
 + old_style_prefix=true
 + else
 + old_style_prefix=false
 + fi

 I don't know if this is a good idea or not, but:

 old_style_prefix=$reuse_branch

It saves a few lines. I'll use that construct in v3.

  fi

  _main $@
 diff --git a/regression/scaffold b/regression/scaffold
 index e4d7487..e4d2f35 100644
 --- a/regression/scaffold
 +++ b/regression/scaffold
 @@ -93,6 +93,7 @@ function setup_git_repo
   git config log.date default
   git config log.decorate no
   git config guilt.diffstat false
 + git config guilt.reusebranch false
  }

  function setup_guilt_repo
 ...
 diff --git a/regression/t-062.sh b/regression/t-062.sh
 new file mode 100755
 index 000..85596ca
 --- /dev/null
 +++ b/regression/t-062.sh
 @@ -0,0 +1,137 @@
 ...

Hidden here was a broken comment.  The new one at the start
of the file will say:

# Test that the guilt.reusebranch=true setting works.

This entire file is mostly a copy of t-061.sh, but slightly
adjusted to use guilt.reusebranch=true.  I'm not sure it
covers all that should be tested. On the other hand, I'm
not sure how much that setting needs to be tested.

 +function fixup_time_info
 +{
 + touch -a -m -t $TOUCH_DATE .git/patches/master/$1
 +}
 +
 +cmd setup_repo
 +
 +cmd git config guilt.reusebranch true
 +
 +cmd guilt push -a
 +cmd list_files
 +cmd git for-each-ref
 +
 +cmd git for-each-ref
 +
 +cmd list_files

 duplicate list_files  for-each-ref

Fixed.

 +
 +for i in `seq 5`; do
 + if [ $i -ge 5 ]; then
 + shouldfail guilt pop
 + else
 + cmd guilt pop
 + fi
 + cmd git for-each-ref
 + cmd guilt push
 + cmd git for-each-ref
 + cmd guilt pop
 + cmd git for-each-ref
 +done
 +
 +# Check that pop -a does the right thing.

 What exactly is the right thing?  no-op since the above loop poped
 everything?  (I'd make the comment say what the right thing is.)

I'll rephrase that block of code like this:

# Check that pop -a properly pops all patches.
cmd guilt push -a
cmd git for-each-ref
cmd guilt pop -a
cmd git for-each-ref

Is that more clear? The test pushes all patches, checks that they
are applied, removes them, checks that it worked.

 Jeff.

 --
 In personal conversations with technical people, I call myself a hacker. But
 when I'm talking to journalists I just say programmer or something like
 that.
 - Linus Torvalds
--
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: Please pull the patch series use the $( ... ) construct for command substitution

[Matthieu Moy]

Eric Sunshine sunsh...@sunshineco.com writes:

 On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy

 I do not understand the use of the \ in front of the ` in the original
 code.

 The second argument of test_expect_success is double-quoted, so a bare
 `...` would be evaluated before test_expect_success is even invoked.
 Quoting it as \'...\' correctly suppresses the automatic evaluation,
 giving test_expect_success the opportunity to evaluate it on-demand.

Ah, of course, you're right.

 The correct code should be

 test x\$(sed -n -e 4p  file)\ = x4 

 I guess.

 Same issue. The $(...) is being evaluated even before
 test_expect_success is invoked. Better would be to suspend evaluation
 via \$(...) to allow test_expect_success to evaluate it on-demand.

OK, then the correct code should be

test x\\$(sed -n -e 4p  file)\ = x4 

And the other attempt was close:

   test_expect_success 'verify pre-merge ancestry' 
  -   test x\`git rev-parse --verify refs/heads/svn^2\` = \
  -x\`git rev-parse --verify refs/heads/merge\` 
  +   test x\$(git rev-parse --verify refs/heads/svn^2\) = \
  +x\$(git rev-parse --verify refs/heads/merge\) 

\-escaping the $ was right, but the \) should be a single ).

In any case, the fact that we need to discuss this supports my claim
this should be reviewed as a separate series.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 0/6] build argv_array into run-command

[Jeff King]

The memory ownership of the argv array of a struct child_process can
be tricky. The child_process does not own the memory, but it must remain
valid until finish_command runs. That's easy for cases where we call
start_command and finish_command in the same function: you can use a
local array variable, or use an argv_array and cleanup afterwards.

But it's easy to screw up in cases where you want to start a command in
one function and finish it in another, either by pointing to invalid
storage during finish_command, or by leaking dynamically allocated
memory.

This series sticks an argv_array inside the struct child_process,
which we clean up automatically.  Because some callers might not want to
use it, it's optional. If you provide argv, we use that, and
otherwise fall back to the internal array.

The first commit below does that. The second fixes an uninitialized
memory access. 3, 4, and 5 plug memory leaks. 6 is just a cleanup for
consistency with the changes in 4 and 5.

And in 2, 3, and 5 we are introducing argv_array into new spots, which
simplifies the code and gets rid of magic numbers.

  [1/6]: run-command: store an optional argv_array
  [2/6]: run_column_filter: use argv_array
  [3/6]: git_connect: use argv_array
  [4/6]: get_helper: use run-command's internal argv_array
  [5/6]: get_exporter: use argv_array
  [6/6]: get_importer: use run-command's internal argv_array

-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

[PATCH 1/6] run-command: store an optional argv_array

[Jeff King]

All child_process structs need to point to an argv. For
flexibility, we do not mandate the use of a dynamic
argv_array. However, because the child_process does not own
the memory, this can make memory management with a
separate argv_array difficult.

For example, if a function calls start_command but not
finish_command, the argv memory must persist. The code needs
to arrange to clean up the argv_array separately after
finish_command runs. As a result, some of our code in this
situation just leaks the memory.

To help such cases, this patch adds a built-in argv_array to
the child_process, which gets cleaned up automatically (both
in finish_command and when start_command fails).  Callers
may use it if they choose, but can continue to use the raw
argv if they wish.

Signed-off-by: Jeff King p...@peff.net
---
This is the most RFC part of the series, because I really didn't know
what to call it. We have two arrays in the struct now: the argv array
which can point to anything, and the new internal argv_array struct,
which here I called args. That name seems confusingly similar; I
wanted something short since it will be used in a lot of calls, but
couldn't think of anything better. Suggestions welcome.

I did toy with the idea of just forcing all callers to use the
argv_array, and calling it argv. But that seemed unnecessarily
disruptive.

 Documentation/technical/api-run-command.txt | 7 +++
 run-command.c   | 9 -
 run-command.h   | 3 +++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/api-run-command.txt 
b/Documentation/technical/api-run-command.txt
index 5d7d7f2..69510ae 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -109,6 +109,13 @@ terminated), of which .argv[0] is the program name to run 
(usually
 without a path). If the command to run is a git command, set argv[0] to
 the command name without the 'git-' prefix and set .git_cmd = 1.
 
+Note that the ownership of the memory pointed to by .argv stays with the
+caller, but it should survive until `finish_command` completes. If the
+.argv member is NULL, `start_command` will point it at the .args
+`argv_array` (so you may use one or the other, but you must use exactly
+one). The memory in .args will be cleaned up automatically during
+`finish_command` (or during `start_command` when it is unsuccessful).
+
 The members .in, .out, .err are used to redirect stdin, stdout,
 stderr as follows:
 
diff --git a/run-command.c b/run-command.c
index 75abc47..be07d4a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -279,6 +279,9 @@ int start_command(struct child_process *cmd)
int failed_errno;
char *str;
 
+   if (!cmd-argv)
+   cmd-argv = cmd-args.argv;
+
/*
 * In case of errors we must keep the promise to close FDs
 * that have been passed in via -in and -out.
@@ -328,6 +331,7 @@ int start_command(struct child_process *cmd)
 fail_pipe:
error(cannot create %s pipe for %s: %s,
str, cmd-argv[0], strerror(failed_errno));
+   argv_array_clear(cmd-args);
errno = failed_errno;
return -1;
}
@@ -519,6 +523,7 @@ fail_pipe:
close_pair(fderr);
else if (cmd-err)
close(cmd-err);
+   argv_array_clear(cmd-args);
errno = failed_errno;
return -1;
}
@@ -543,7 +548,9 @@ fail_pipe:
 
 int finish_command(struct child_process *cmd)
 {
-   return wait_or_whine(cmd-pid, cmd-argv[0]);
+   int ret = wait_or_whine(cmd-pid, cmd-argv[0]);
+   argv_array_clear(cmd-args);
+   return ret;
 }
 
 int run_command(struct child_process *cmd)
diff --git a/run-command.h b/run-command.h
index 3653bfa..ea73de3 100644
--- a/run-command.h
+++ b/run-command.h
@@ -5,8 +5,11 @@
 #include pthread.h
 #endif
 
+#include argv-array.h
+
 struct child_process {
const char **argv;
+   struct argv_array args;
pid_t pid;
/*
 * Using .in, .out, .err:
-- 
2.0.0.rc1.436.g03cb729

--
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/6] run_column_filter: use argv_array

[Jeff King]

We currently set up the argv array by hand in a fixed-size
stack-local array. Using an argv array is more readable, as
it handles buffer allocation us (not to mention makes it
obvious we do not overflow the array).

However, there's a more subtle benefit, too. We leave the
function having run start_command (with the child_process
in a static global), and then later run finish_command from
another function. That means when we run finish_command,
neither column_process.argv nor the memory it points to is
valid any longer.

Most of the time finish_command does not bother looking at
argv, but it may if it encounters an error (e.g., waitpid
failure or signal death). This is unusual, which is why
nobody has noticed. But by using run-command's built-in
argv_array, the memory ownership is handled for us
automatically.

Signed-off-by: Jeff King p...@peff.net
---
 column.c | 43 +--
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/column.c b/column.c
index 8d1ce88..1a468de 100644
--- a/column.c
+++ b/column.c
@@ -370,46 +370,29 @@ static struct child_process column_process;
 
 int run_column_filter(int colopts, const struct column_options *opts)
 {
-   const char *av[10];
-   int ret, ac = 0;
-   struct strbuf sb_colopt  = STRBUF_INIT;
-   struct strbuf sb_width   = STRBUF_INIT;
-   struct strbuf sb_padding = STRBUF_INIT;
+   struct argv_array *argv;
 
if (fd_out != -1)
return -1;
 
-   av[ac++] = column;
-   strbuf_addf(sb_colopt, --raw-mode=%d, colopts);
-   av[ac++] = sb_colopt.buf;
-   if (opts  opts-width) {
-   strbuf_addf(sb_width, --width=%d, opts-width);
-   av[ac++] = sb_width.buf;
-   }
-   if (opts  opts-indent) {
-   av[ac++] = --indent;
-   av[ac++] = opts-indent;
-   }
-   if (opts  opts-padding) {
-   strbuf_addf(sb_padding, --padding=%d, opts-padding);
-   av[ac++] = sb_padding.buf;
-   }
-   av[ac] = NULL;
+   memset(column_process, 0, sizeof(column_process));
+   argv = column_process.args;
+
+   argv_array_push(argv, column);
+   argv_array_pushf(argv, --raw-mode=%d, colopts);
+   if (opts  opts-width)
+   argv_array_pushf(argv, --width=%d, opts-width);
+   if (opts  opts-indent)
+   argv_array_pushf(argv, --indent=%s, opts-indent);
+   if (opts  opts-padding)
+   argv_array_pushf(argv, --padding=%d, opts-padding);
 
fflush(stdout);
-   memset(column_process, 0, sizeof(column_process));
column_process.in = -1;
column_process.out = dup(1);
column_process.git_cmd = 1;
-   column_process.argv = av;
-
-   ret = start_command(column_process);
-
-   strbuf_release(sb_colopt);
-   strbuf_release(sb_width);
-   strbuf_release(sb_padding);
 
-   if (ret)
+   if (start_command(column_process))
return -2;
 
fd_out = dup(1);
-- 
2.0.0.rc1.436.g03cb729

--
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 5/6] get_exporter: use argv_array

[Jeff King]

This simplifies the code and avoids a fixed array size that
we might accidentally overflow. It also prevents a leak
after finish_command is run, by using the argv_array that
run-command manages for us.

Signed-off-by: Jeff King p...@peff.net
---
 transport-helper.c | 26 ++
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index fefd34f..9f8f3b1 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -418,30 +418,24 @@ static int get_exporter(struct transport *transport,
 {
struct helper_data *data = transport-data;
struct child_process *helper = get_helper(transport);
-   int argc = 0, i;
-   struct strbuf tmp = STRBUF_INIT;
+   int i;
 
memset(fastexport, 0, sizeof(*fastexport));
 
/* we need to duplicate helper-in because we want to use it after
 * fastexport is done with it. */
fastexport-out = dup(helper-in);
-   fastexport-argv = xcalloc(6 + revlist_args-nr, 
sizeof(*fastexport-argv));
-   fastexport-argv[argc++] = fast-export;
-   fastexport-argv[argc++] = --use-done-feature;
-   fastexport-argv[argc++] = data-signed_tags ?
-   --signed-tags=verbatim : --signed-tags=warn-strip;
-   if (data-export_marks) {
-   strbuf_addf(tmp, --export-marks=%s.tmp, data-export_marks);
-   fastexport-argv[argc++] = strbuf_detach(tmp, NULL);
-   }
-   if (data-import_marks) {
-   strbuf_addf(tmp, --import-marks=%s, data-import_marks);
-   fastexport-argv[argc++] = strbuf_detach(tmp, NULL);
-   }
+   argv_array_push(fastexport-args, fast-export);
+   argv_array_push(fastexport-args, --use-done-feature);
+   argv_array_push(fastexport-args, data-signed_tags ?
+   --signed-tags=verbatim : --signed-tags=warn-strip);
+   if (data-export_marks)
+   argv_array_pushf(fastexport-args, --export-marks=%s.tmp, 
data-export_marks);
+   if (data-import_marks)
+   argv_array_pushf(fastexport-args, --import-marks=%s, 
data-import_marks);
 
for (i = 0; i  revlist_args-nr; i++)
-   fastexport-argv[argc++] = revlist_args-items[i].string;
+   argv_array_push(fastexport-args, 
revlist_args-items[i].string);
 
fastexport-git_cmd = 1;
return start_command(fastexport);
-- 
2.0.0.rc1.436.g03cb729

--
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 3/6] git_connect: use argv_array

[Jeff King]

This avoids magic numbers when we allocate fixed-size argv
arrays, and makes it more obvious that we are not
overflowing.

It is also the first step to fixing a memory leak. When
git_connect returns a child_process struct, the argv array
in the struct is dynamically allocated, but the individual
strings are not (they are either owned elsewhere, or are
freed). Later, in finish_connect, we free the array but
leave the strings alone.

This works for the child_process created by git_connect, but
if we use transport_take_over, we may also end up with a
child_process created by transport-helper's get_helper.
In that case, the strings are freshly allocated, and we
would want to free them.  However, we have no idea in
finish_connect which type we have.

By consistently using run-command's internal argv-array, we
do not have to worry about this issue at all; finish_command
takes care of it for us, and we can drop our manual free
entirely.

Note that this actually makes the get_helper leak slightly
worse; now we are leaking both the strings and the array.
But when we adjust it in a future patch, that leak will go
away entirely.

Signed-off-by: Jeff King p...@peff.net
---
 connect.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/connect.c b/connect.c
index a983d06..94a6650 100644
--- a/connect.c
+++ b/connect.c
@@ -534,22 +534,18 @@ static int git_use_proxy(const char *host)
 static struct child_process *git_proxy_connect(int fd[2], char *host)
 {
const char *port = STR(DEFAULT_GIT_PORT);
-   const char **argv;
struct child_process *proxy;
 
get_host_and_port(host, port);
 
-   argv = xmalloc(sizeof(*argv) * 4);
-   argv[0] = git_proxy_command;
-   argv[1] = host;
-   argv[2] = port;
-   argv[3] = NULL;
proxy = xcalloc(1, sizeof(*proxy));
-   proxy-argv = argv;
+   argv_array_push(proxy-args, git_proxy_command);
+   argv_array_push(proxy-args, host);
+   argv_array_push(proxy-args, port);
proxy-in = -1;
proxy-out = -1;
if (start_command(proxy))
-   die(cannot start proxy %s, argv[0]);
+   die(cannot start proxy %s, git_proxy_command);
fd[0] = proxy-out; /* read from proxy stdout */
fd[1] = proxy-in;  /* write to proxy stdin */
return proxy;
@@ -663,7 +659,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
char *hostandport, *path;
struct child_process *conn = no_fork;
enum protocol protocol;
-   const char **arg;
struct strbuf cmd = STRBUF_INIT;
 
/* Without this we cannot rely on waitpid() to tell
@@ -707,7 +702,6 @@ struct child_process *git_connect(int fd[2], const char 
*url,
sq_quote_buf(cmd, path);
 
conn-in = conn-out = -1;
-   conn-argv = arg = xcalloc(7, sizeof(*arg));
if (protocol == PROTO_SSH) {
const char *ssh = getenv(GIT_SSH);
int putty = ssh  strcasestr(ssh, plink);
@@ -718,22 +712,21 @@ struct child_process *git_connect(int fd[2], const char 
*url,
 
if (!ssh) ssh = ssh;
 
-   *arg++ = ssh;
+   argv_array_push(conn-args, ssh);
if (putty  !strcasestr(ssh, tortoiseplink))
-   *arg++ = -batch;
+   argv_array_push(conn-args, -batch);
if (port) {
/* P is for PuTTY, p is for OpenSSH */
-   *arg++ = putty ? -P : -p;
-   *arg++ = port;
+   argv_array_push(conn-args, putty ? -P : 
-p);
+   argv_array_push(conn-args, port);
}
-   *arg++ = ssh_host;
+   argv_array_push(conn-args, ssh_host);
} else {
/* remove repo-local variables from the environment */
conn-env = local_repo_env;
conn-use_shell = 1;
}
-   *arg++ = cmd.buf;
-   *arg = NULL;
+   argv_array_push(conn-args, cmd.buf);
 
if (start_command(conn))
die(unable to fork);
@@ -759,7 +752,6 @@ int finish_connect(struct child_process *conn)
return 0;
 
code = finish_command(conn);
-   free(conn-argv);
free(conn);
return code;
 }
-- 
2.0.0.rc1.436.g03cb729

--
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 4/6] get_helper: use run-command's internal argv_array

[Jeff King]

The get_helper functions dynamically allocates an
argv_array, feeds it to start_command, and then returns. We
then have to later clean up the memory manually after
calling finish_command. We can make this simpler by just
using run-command's internal argv_array, which handles
cleanup for us.

This also prevents a memory leak in the case that
transport_take_over is used, in which case we free the child
in finish_connect, which does not manually free the array.

Signed-off-by: Jeff King p...@peff.net
---
 transport-helper.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b468e4f..fefd34f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -101,7 +101,6 @@ static void do_take_over(struct transport *transport)
 static struct child_process *get_helper(struct transport *transport)
 {
struct helper_data *data = transport-data;
-   struct argv_array argv = ARGV_ARRAY_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process *helper;
const char **refspecs = NULL;
@@ -123,10 +122,9 @@ static struct child_process *get_helper(struct transport 
*transport)
helper-in = -1;
helper-out = -1;
helper-err = 0;
-   argv_array_pushf(argv, git-remote-%s, data-name);
-   argv_array_push(argv, transport-remote-name);
-   argv_array_push(argv, remove_ext_force(transport-url));
-   helper-argv = argv_array_detach(argv, NULL);
+   argv_array_pushf(helper-args, git-remote-%s, data-name);
+   argv_array_push(helper-args, transport-remote-name);
+   argv_array_push(helper-args, remove_ext_force(transport-url));
helper-git_cmd = 0;
helper-silent_exec_failure = 1;
 
@@ -245,7 +243,6 @@ static int disconnect_helper(struct transport *transport)
close(data-helper-out);
fclose(data-out);
res = finish_command(data-helper);
-   argv_array_free_detached(data-helper-argv);
free(data-helper);
data-helper = NULL;
}
-- 
2.0.0.rc1.436.g03cb729

--
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 6/6] get_importer: use run-command's internal argv_array

[Jeff King]

This saves a few lines and lets us avoid having to clean up
the memory manually when the command finishes.

Signed-off-by: Jeff King p...@peff.net
---
 transport-helper.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 9f8f3b1..d9063d7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -394,18 +394,16 @@ static int get_importer(struct transport *transport, 
struct child_process *fasti
 {
struct child_process *helper = get_helper(transport);
struct helper_data *data = transport-data;
-   struct argv_array argv = ARGV_ARRAY_INIT;
int cat_blob_fd, code;
memset(fastimport, 0, sizeof(*fastimport));
fastimport-in = helper-out;
-   argv_array_push(argv, fast-import);
-   argv_array_push(argv, debug ? --stats : --quiet);
+   argv_array_push(fastimport-args, fast-import);
+   argv_array_push(fastimport-args, debug ? --stats : --quiet);
 
if (data-bidi_import) {
cat_blob_fd = xdup(helper-in);
-   argv_array_pushf(argv, --cat-blob-fd=%d, cat_blob_fd);
+   argv_array_pushf(fastimport-args, --cat-blob-fd=%d, 
cat_blob_fd);
}
-   fastimport-argv = argv.argv;
fastimport-git_cmd = 1;
 
code = start_command(fastimport);
@@ -476,7 +474,6 @@ static int fetch_with_import(struct transport *transport,
 
if (finish_command(fastimport))
die(Error while running fast-import);
-   argv_array_free_detached(fastimport.argv);
 
/*
 * The fast-import stream of a remote helper that advertises
-- 
2.0.0.rc1.436.g03cb729
--
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 7/6] argv-array: drop detach code

[Jeff King]

The argv_array_detach function (and associated free() function) was
really only useful for transferring ownership of the memory to a struct
child_process. Now that we have an internal argv_array in that struct,
there are no callers left.

Signed-off-by: Jeff King p...@peff.net
---
This is a bonus enabled by the earlier patches. However, there is one
commit in pu that uses it when dealing with environment variables.
However, it is actually leaking memory, and should probably just use the
array directly (and it's one of my commits that's due to be re-rolled
anyway).

 Documentation/technical/api-argv-array.txt |  8 
 argv-array.c   | 20 
 argv-array.h   |  2 --
 3 files changed, 30 deletions(-)

diff --git a/Documentation/technical/api-argv-array.txt 
b/Documentation/technical/api-argv-array.txt
index a6b7d83..1a79781 100644
--- a/Documentation/technical/api-argv-array.txt
+++ b/Documentation/technical/api-argv-array.txt
@@ -53,11 +53,3 @@ Functions
 `argv_array_clear`::
Free all memory associated with the array and return it to the
initial, empty state.
-
-`argv_array_detach`::
-   Detach the argv array from the `struct argv_array`, transferring
-   ownership of the allocated array and strings.
-
-`argv_array_free_detached`::
-   Free the memory allocated by a `struct argv_array` that was later
-   detached and is now no longer needed.
diff --git a/argv-array.c b/argv-array.c
index 9e960d5..256741d 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -68,23 +68,3 @@ void argv_array_clear(struct argv_array *array)
}
argv_array_init(array);
 }
-
-const char **argv_array_detach(struct argv_array *array, int *argc)
-{
-   const char **argv =
-   array-argv == empty_argv || array-argc == 0 ? NULL : 
array-argv;
-   if (argc)
-   *argc = array-argc;
-   argv_array_init(array);
-   return argv;
-}
-
-void argv_array_free_detached(const char **argv)
-{
-   if (argv) {
-   int i;
-   for (i = 0; argv[i]; i++)
-   free((char **)argv[i]);
-   free(argv);
-   }
-}
diff --git a/argv-array.h b/argv-array.h
index 85ba438..c65e6e8 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,7 +19,5 @@ LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
-const char **argv_array_detach(struct argv_array *array, int *argc);
-void argv_array_free_detached(const char **argv);
 
 #endif /* ARGV_ARRAY_H */
-- 
2.0.0.rc1.436.g03cb729
--
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] open_sha1_file: report most interesting errno

[Jeff King]

When we try to open a loose object file, we first attempt to
open in the local object database, and then try any
alternates. This means that the errno value when we return
will be from the last place we looked (and due to the way
the code is structured, simply ENOENT if we do not have have
any alternates).

This can cause confusing error messages, as read_sha1_file
checks for ENOENT when reporting a missing object. If errno
is something else, we report that. If it is ENOENT, but
has_loose_object reports that we have it, then we claim the
object is corrupted. For example:

$ chmod 0 .git/objects/??/*
$ git rev-list --all
fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in 
.git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt

This patch instead keeps track of the most interesting
errno we receive during our search. We consider ENOENT to be
the least interesting of all, and otherwise report the first
error found (so problems in the object database take
precedence over ones in alternates). Here it is with this
patch:

$ git rev-list --all
fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: 
Permission denied

Signed-off-by: Jeff King p...@peff.net
---
 sha1_file.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 3e9f55f..34d527f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1437,19 +1437,23 @@ static int open_sha1_file(const unsigned char *sha1)
 {
int fd;
struct alternate_object_database *alt;
+   int most_interesting_errno;
 
fd = git_open_noatime(sha1_file_name(sha1));
if (fd = 0)
return fd;
+   most_interesting_errno = errno;
 
prepare_alt_odb();
-   errno = ENOENT;
for (alt = alt_odb_list; alt; alt = alt-next) {
fill_sha1_path(alt-name, sha1);
fd = git_open_noatime(alt-base);
if (fd = 0)
return fd;
+   if (most_interesting_errno == ENOENT)
+   most_interesting_errno = errno;
}
+   errno = most_interesting_errno;
return -1;
 }
 
-- 
2.0.0.rc1.436.g03cb729
--
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] transport-helper: add trailing --

[Jeff King]

On Thu, May 15, 2014 at 07:32:14AM +0200, Stepan Kasal wrote:

 From: Sverre Rabbelier srabbel...@gmail.com
 Date: Sat, 28 Aug 2010 20:49:01 -0500
 
 [PT: ensure we add an additional element to the argv array]
 
 Signed-off-by: Stepan Kasal ka...@ucw.cz
 ---
 
 Hi,
   this patch was present in msysgit from Mar 2012.
 Do you like it?
 I'm sorry, there is no author signoff; is the patch small enough?

It needs an explanation in the commit message, too. As Felipe noted, I
do not think it would help with ambiguity, but it should not hurt, and
is a reasonable defensive thing to do (but I did not think about it too
long, so maybe Sverre has an example that needs it).

 diff --git a/transport-helper.c b/transport-helper.c
 index 0e7c330..a01ea47 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c
 @@ -429,7 +429,7 @@ static int get_exporter(struct transport *transport,
   /* we need to duplicate helper-in because we want to use it after
* fastexport is done with it. */
   fastexport-out = dup(helper-in);
 - fastexport-argv = xcalloc(6 + revlist_args-nr, 
 sizeof(*fastexport-argv));
 + fastexport-argv = xcalloc(7 + revlist_args-nr, 
 sizeof(*fastexport-argv));

It would be nice if this were an argv_array so we would not have to
worry about managing the array size. This is one of several spots that
leaks array memory that I have been meaning to fix.

I just posted a series that addresses those leaks and converts this
site. I do not want to hold your patch hostage to my series, but
depending on the review on my series, you may want to re-roll this on
top; you would drop the line above, and change:

 + fastexport-argv[argc++] = --;

to:

argv_array_push(fastexport-args, --);

-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/RFC] Gitweb: Convert UTF-8 encoded file names

[Peter Krefting]

Michael Wagner:


Decoding the UTF-8 encoded file name (again with an additional print
statement):

$ REQUEST_METHOD=GET 
QUERY_STRING='p=notes.git;a=blob_plain;f=work/G%C3%83%C2%BCtekriterien.txt;hb=HEAD'
 ./gitweb.cgi

work/Gütekriterien.txt
Content-disposition: inline; filename=work/Gütekriterien.txt


You should fix the code path that created that URI, though, as it is 
not what you expected.


%C3%83 decodes to U+00C3 Latin Capital Letter A With Tilde
%C2%BC decodes to U+00BC Vulgar Graction One Quarter

The proper UTF-8 encoding for ü (U+00FC) is, as you can probably guess from 
looking at which two characters the sequence above yielded, C3 BC, 
which in a URI is represented as %C3%BC.


Your QUERY_STRING should thus be

  p=notes.git;a=blob_plain;f=work/G%C3%BCtekriterien.txt;hb=HEAD

which probably works as expected.

What is happening is that whatever is generating the URI us 
UTF-8-encoding the string twice (i.e., it generates a string with the 
proper C3 BC in it, and then interprets it as iso-8859-1 data and runs 
that through a UTF-8 encoder again, yielding the C3 83 C2 BC sequence 
you see above).


--
\\// Peter - http://www.softwolves.pp.se/
--
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] transport-helper: add trailing --

[Stepan Kasal]

Hi Peff,

On Thu, May 15, 2014 at 05:00:13AM -0400, Jeff King wrote:
 I just posted a series that addresses those leaks and converts this
 site. I do not want to hold your patch hostage to my series, [...]

no problem, let's return to it later.

Stepan
--
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/RFC] Gitweb: Convert UTF-8 encoded file names

[Jakub Narębski]

On Thu, May 15, 2014 at 7:08 AM, Michael Wagner accou...@mwagner.org wrote:
 On Thu, May 15, 2014 at 12:25:45AM +0200, Jakub Narębski wrote:
 On Wed, May 14, 2014 at 11:57 PM, Junio C Hamano gits...@pobox.com wrote:
 Michael Wagner accou...@mwagner.org writes:

 Perl has an internal encoding used to store text strings. Currently, 
 trying to
 view files with UTF-8 encoded names results in an error (either 404 - 
 Cannot
 find file [blob_plain] or XML Parsing Error [blob]). Converting these 
 UTF-8
 encoded file names into Perl's internal format resolves these errors.

 Could you give us an example?  What is important is whether filename
 is passed via path_info or via query string.


 There is a file named Gütekriterien.txt in my repository. Trying to
 view this file as blob_plain produces an 404 error (displaying the
 file name with an additional print statement):

 $ REQUEST_METHOD=GET 
 QUERY_STRING='p=notes.git;a=blob_plain;f=work/G%C3%83%C2%BCtekriterien.txt;hb=HEAD'
  ./gitweb.cgi

 work/Gütekriterien.txt
 Status: 404 Not Found

You have URI encoding of ü wrong! ü encodes as %C3%BC, not
as %C3%83%C2%BC (4 bytes?)

  http://www.url-encode-decode.com/

You tested with wrong input.

BTW. there probably should be test for UTF-8 encoding, similar to
the one for XSS in t9502-gitweb-standalone-parse-output
-- 
Jakub Narębski
--
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 09/10] git-rebase--interactive.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-rebase--interactive.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6ec9d3c..797571f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1013,7 +1013,7 @@ then
git rev-list $revisions |
while read rev
do
-   if test -f $rewritten/$rev -a $(sane_grep $rev 
$state_dir/not-cherry-picks) = 
+   if test -f $rewritten/$rev  test $(sane_grep $rev 
$state_dir/not-cherry-picks) = 
then
# Use -f2 because if rev-list is telling us this commit 
is
# not worthwhile, we don't want to track its multiple 
heads,
-- 
1.7.10.4

--
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 03/10] contrib/examples/git-commit.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-commit.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
index 5cafe2e..934505b 100755
--- a/contrib/examples/git-commit.sh
+++ b/contrib/examples/git-commit.sh
@@ -51,7 +51,7 @@ run_status () {
export GIT_INDEX_FILE
fi
 
-   if test $status_only = t -o $use_status_color = t; then
+   if test $status_only = t || test $use_status_color = t; then
color=
else
color=--nocolor
@@ -296,7 +296,7 @@ t,,,[1-9]*)
die No paths with -i does not make sense. ;;
 esac
 
-if test ! -z $templatefile -a -z $log_given
+if test ! -z $templatefile  test -z $log_given
 then
if test ! -f $templatefile
then
-- 
1.7.10.4

--
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 07/10] git-bisect.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-bisect.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index af4d04c..1e0d602 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -408,7 +408,7 @@ bisect_replay () {
bisect_reset
while read git bisect command rev
do
-   test $git $bisect = git bisect -o $git = git-bisect || 
continue
+   test $git $bisect = git bisect || test $git = 
git-bisect || continue
if test $git = git-bisect
then
rev=$command
-- 
1.7.10.4

--
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 01/10] check_bindir: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 check_bindir |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check_bindir b/check_bindir
index a1c4c3e..623eadc 100755
--- a/check_bindir
+++ b/check_bindir
@@ -2,7 +2,7 @@
 bindir=$1
 gitexecdir=$2
 gitcmd=$3
-if test $bindir != $gitexecdir -a -x $gitcmd
+if test $bindir != $gitexecdir  test -x $gitcmd
 then
echo
echo !! You have installed git-* commands to new gitexecdir.
-- 
1.7.10.4

--
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 04/10] contrib/examples/git-merge.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-merge.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-merge.sh b/contrib/examples/git-merge.sh
index 7e40f40..52f2aaf 100755
--- a/contrib/examples/git-merge.sh
+++ b/contrib/examples/git-merge.sh
@@ -161,7 +161,7 @@ merge_name () {
return
fi
fi
-   if test $remote = FETCH_HEAD -a -r $GIT_DIR/FETCH_HEAD
+   if test $remote = FETCH_HEAD  test -r $GIT_DIR/FETCH_HEAD
then
sed -e 's/  not-for-merge   /   /' -e 1q \
$GIT_DIR/FETCH_HEAD
@@ -527,7 +527,7 @@ do
git diff-files --name-only
git ls-files --unmerged
} | wc -l`
-   if test $best_cnt -le 0 -o $cnt -le $best_cnt
+   if test $best_cnt -le 0 || test $cnt -le $best_cnt
then
best_strategy=$strategy
best_cnt=$cnt
-- 
1.7.10.4

--
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 05/10] contrib/examples/git-repack.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-repack.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
index f312405..96e3fed 100755
--- a/contrib/examples/git-repack.sh
+++ b/contrib/examples/git-repack.sh
@@ -76,8 +76,8 @@ case ,$all_into_one, in
existing=$existing $e
fi
done
-   if test -n $existing -a -n $unpack_unreachable -a \
-   -n $remove_redundant
+   if test -n $existing  test -n $unpack_unreachable  \
+   test -n $remove_redundant
then
# This may have arbitrary user arguments, so we
# have to protect it against whitespace splitting
-- 
1.7.10.4

--
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 02/10] contrib/examples/git-clone.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-clone.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-clone.sh b/contrib/examples/git-clone.sh
index b4c9376..08cf246 100755
--- a/contrib/examples/git-clone.sh
+++ b/contrib/examples/git-clone.sh
@@ -516,7 +516,7 @@ then
 
case $no_checkout in
'')
-   test z$quiet = z -a z$no_progress = z  v=-v || v=
+   test z$quiet = z  test z$no_progress = z  v=-v || v=
git read-tree -m -u $v HEAD HEAD
esac
 fi
-- 
1.7.10.4

--
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 08/10] git-mergetool.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-mergetool.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 332528f..88e853f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -205,7 +205,7 @@ checkout_staged_file () {
$(git checkout-index --temp --stage=$1 $2 2/dev/null) \
: '\([^ ]*\)')
 
-   if test $? -eq 0 -a -n $tmpfile
+   if test $? -eq 0  test -n $tmpfile
then
mv -- $(git rev-parse --show-cdup)$tmpfile $3
else
@@ -256,7 +256,7 @@ merge_file () {
checkout_staged_file 2 $MERGED $LOCAL
checkout_staged_file 3 $MERGED $REMOTE
 
-   if test -z $local_mode -o -z $remote_mode
+   if test -z $local_mode || test -z $remote_mode
then
echo Deleted merge conflict for '$MERGED':
describe_file $local_mode local $LOCAL
-- 
1.7.10.4

--
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 10/10] git-submodule.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-submodule.sh |   14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..d89e1d0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?)
then
subforce=$force
# If we don't already have a -f flag and the submodule 
has never been checked out
-   if test -z $subsha1 -a -z $force
+   if test -z $subsha1 || test -z $force
then
subforce=-f
fi
@@ -1059,13 +1059,13 @@ cmd_summary() {
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
# Always show modules deleted or type-changed 
(blob-module)
-   test $status = D -o $status = T  echo $sm_path  
continue
+( test $status = D || test $status = T )  echo 
$sm_path  continue
# Respect the ignore setting for --for-status.
if test -n $for_status
then
name=$(module_name $sm_path)
ignore_config=$(get_submodule_config $name 
ignore none)
-   test $status != A -a $ignore_config = all  
continue
+   test $status != A  test $ignore_config = all 
 continue
fi
# Also show added or modified modules which are checked 
out
GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
/dev/null 21 
@@ -1125,7 +1125,7 @@ cmd_summary() {
*)
errmsg=
total_commits=$(
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
range=$sha1_src...$sha1_dst
elif test $mod_src = 16
@@ -1162,7 +1162,7 @@ cmd_summary() {
# i.e. deleted or changed to blob
test $mod_dst = 16  echo $errmsg
else
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
limit=
test $summary_limit -gt 0  
limit=-$summary_limit
@@ -1233,7 +1233,7 @@ cmd_status()
say U$sha1 $displaypath
continue
fi
-   if test -z $url || ! test -d $sm_path/.git -o -f 
$sm_path/.git
+   if test -z $url || ! test -d $sm_path/.git || test -f 
$sm_path/.git
then
say -$sha1 $displaypath
continue;
@@ -1402,7 +1402,7 @@ then
 fi
 
 # --cached is accepted only by status and summary
-if test -n $cached  test $command != status -a $command != summary
+if test -n $cached  test $command != status  test $command != summary
 then
usage
 fi
-- 
1.7.10.4

--
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 06/10] contrib/examples/git-resolve.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-resolve.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-resolve.sh b/contrib/examples/git-resolve.sh
index 48d0fc9..70fdc27 100755
--- a/contrib/examples/git-resolve.sh
+++ b/contrib/examples/git-resolve.sh
@@ -76,7 +76,7 @@ case $common in
2/dev/null || continue
# Count the paths that are unmerged.
cnt=$(GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l)
-   if test $best_cnt -le 0 -o $cnt -le $best_cnt
+   if test $best_cnt -le 0 || test $cnt -le $best_cnt
then
best=$c
best_cnt=$cnt
-- 
1.7.10.4

--
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 10/10] git-submodule.sh: don't use the -a or -b option with the test command

[Elia Pinto]

I am very sorry : the comment is wrong. I will repost shortly.

Best regards

2014-05-15 16:17 GMT+02:00 Elia Pinto gitter.spi...@gmail.com:
 Even though POSIX.1 lists -a/-o as options to test, they are
 marked Obsolescent XSI. Scripts using these expressions
 should be converted  as follow:

 test $1 -a $2

 should be written as:

 test $1  test $2

 Likewise

 test $1 -o $2

 should be written as:

 test $1  test $2

 But note that, in test, -a has higher precedence than -o while
  and || have equal precedence in the shell.

 The reason for this is that the precedence rules were never well
 specified, and this made many sane-looking uses of test -a/-o problematic.

 For example, if $x is =, these work according to POSIX (it's not
 portable, but in practice it's okay):

$ test -z $x
$ test -z $x  test a = b

 but this doesn't

$ test -z $x -a a = b
bash: test: too many arguments

 because it groups test -n = -a and is left with a = b.

 Similarly, if $x is -f, these

$ test $x
$ test $x || test c = d

 correctly adds an implicit -n, but this fails:

$ test $x -o c = d
bash: test: too many arguments

 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
 Inspired from this discussion 
 http://permalink.gmane.org/gmane.comp.version-control.git/137056

  git-submodule.sh |   14 +++---
  1 file changed, 7 insertions(+), 7 deletions(-)

 diff --git a/git-submodule.sh b/git-submodule.sh
 index b55d83a..d89e1d0 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?)
 then
 subforce=$force
 # If we don't already have a -f flag and the 
 submodule has never been checked out
 -   if test -z $subsha1 -a -z $force
 +   if test -z $subsha1 || test -z $force
 then
 subforce=-f
 fi
 @@ -1059,13 +1059,13 @@ cmd_summary() {
 while read mod_src mod_dst sha1_src sha1_dst status sm_path
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 -   test $status = D -o $status = T  echo $sm_path  
 continue
 +( test $status = D || test $status = T )  echo 
 $sm_path  continue
 # Respect the ignore setting for --for-status.
 if test -n $for_status
 then
 name=$(module_name $sm_path)
 ignore_config=$(get_submodule_config $name 
 ignore none)
 -   test $status != A -a $ignore_config = all  
 continue
 +   test $status != A  test $ignore_config = 
 all  continue
 fi
 # Also show added or modified modules which are 
 checked out
 GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
 /dev/null 21 
 @@ -1125,7 +1125,7 @@ cmd_summary() {
 *)
 errmsg=
 total_commits=$(
 -   if test $mod_src = 16 -a $mod_dst = 16
 +   if test $mod_src = 16  test $mod_dst = 16
 then
 range=$sha1_src...$sha1_dst
 elif test $mod_src = 16
 @@ -1162,7 +1162,7 @@ cmd_summary() {
 # i.e. deleted or changed to blob
 test $mod_dst = 16  echo $errmsg
 else
 -   if test $mod_src = 16 -a $mod_dst = 16
 +   if test $mod_src = 16  test $mod_dst = 16
 then
 limit=
 test $summary_limit -gt 0  
 limit=-$summary_limit
 @@ -1233,7 +1233,7 @@ cmd_status()
 say U$sha1 $displaypath
 continue
 fi
 -   if test -z $url || ! test -d $sm_path/.git -o -f 
 $sm_path/.git
 +   if test -z $url || ! test -d $sm_path/.git || test -f 
 $sm_path/.git
 then
 say -$sha1 $displaypath
 continue;
 @@ -1402,7 +1402,7 @@ then
  fi

  # --cached is accepted only by status and summary
 -if test -n $cached  test $command != status -a $command != summary
 +if test -n $cached  test $command != status  test $command != 
 summary
  then
 usage
  fi
 --
 1.7.10.4

--
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 01/10] check_bindir: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 check_bindir |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check_bindir b/check_bindir
index a1c4c3e..623eadc 100755
--- a/check_bindir
+++ b/check_bindir
@@ -2,7 +2,7 @@
 bindir=$1
 gitexecdir=$2
 gitcmd=$3
-if test $bindir != $gitexecdir -a -x $gitcmd
+if test $bindir != $gitexecdir  test -x $gitcmd
 then
echo
echo !! You have installed git-* commands to new gitexecdir.
-- 
1.7.10.4

--
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 05/10] contrib/examples/git-repack.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-repack.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
index f312405..96e3fed 100755
--- a/contrib/examples/git-repack.sh
+++ b/contrib/examples/git-repack.sh
@@ -76,8 +76,8 @@ case ,$all_into_one, in
existing=$existing $e
fi
done
-   if test -n $existing -a -n $unpack_unreachable -a \
-   -n $remove_redundant
+   if test -n $existing  test -n $unpack_unreachable  \
+   test -n $remove_redundant
then
# This may have arbitrary user arguments, so we
# have to protect it against whitespace splitting
-- 
1.7.10.4

--
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 08/10] git-mergetool.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-mergetool.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 332528f..88e853f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -205,7 +205,7 @@ checkout_staged_file () {
$(git checkout-index --temp --stage=$1 $2 2/dev/null) \
: '\([^ ]*\)')
 
-   if test $? -eq 0 -a -n $tmpfile
+   if test $? -eq 0  test -n $tmpfile
then
mv -- $(git rev-parse --show-cdup)$tmpfile $3
else
@@ -256,7 +256,7 @@ merge_file () {
checkout_staged_file 2 $MERGED $LOCAL
checkout_staged_file 3 $MERGED $REMOTE
 
-   if test -z $local_mode -o -z $remote_mode
+   if test -z $local_mode || test -z $remote_mode
then
echo Deleted merge conflict for '$MERGED':
describe_file $local_mode local $LOCAL
-- 
1.7.10.4

--
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 04/10] contrib/examples/git-merge.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-merge.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-merge.sh b/contrib/examples/git-merge.sh
index 7e40f40..52f2aaf 100755
--- a/contrib/examples/git-merge.sh
+++ b/contrib/examples/git-merge.sh
@@ -161,7 +161,7 @@ merge_name () {
return
fi
fi
-   if test $remote = FETCH_HEAD -a -r $GIT_DIR/FETCH_HEAD
+   if test $remote = FETCH_HEAD  test -r $GIT_DIR/FETCH_HEAD
then
sed -e 's/  not-for-merge   /   /' -e 1q \
$GIT_DIR/FETCH_HEAD
@@ -527,7 +527,7 @@ do
git diff-files --name-only
git ls-files --unmerged
} | wc -l`
-   if test $best_cnt -le 0 -o $cnt -le $best_cnt
+   if test $best_cnt -le 0 || test $cnt -le $best_cnt
then
best_strategy=$strategy
best_cnt=$cnt
-- 
1.7.10.4

--
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 03/10] contrib/examples/git-commit.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-commit.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
index 5cafe2e..934505b 100755
--- a/contrib/examples/git-commit.sh
+++ b/contrib/examples/git-commit.sh
@@ -51,7 +51,7 @@ run_status () {
export GIT_INDEX_FILE
fi
 
-   if test $status_only = t -o $use_status_color = t; then
+   if test $status_only = t || test $use_status_color = t; then
color=
else
color=--nocolor
@@ -296,7 +296,7 @@ t,,,[1-9]*)
die No paths with -i does not make sense. ;;
 esac
 
-if test ! -z $templatefile -a -z $log_given
+if test ! -z $templatefile  test -z $log_given
 then
if test ! -f $templatefile
then
-- 
1.7.10.4

--
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 06/10] contrib/examples/git-resolve.sh: don't use the -a or -b option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-resolve.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-resolve.sh b/contrib/examples/git-resolve.sh
index 48d0fc9..70fdc27 100755
--- a/contrib/examples/git-resolve.sh
+++ b/contrib/examples/git-resolve.sh
@@ -76,7 +76,7 @@ case $common in
2/dev/null || continue
# Count the paths that are unmerged.
cnt=$(GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l)
-   if test $best_cnt -le 0 -o $cnt -le $best_cnt
+   if test $best_cnt -le 0 || test $cnt -le $best_cnt
then
best=$c
best_cnt=$cnt
-- 
1.7.10.4

--
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 09/10] git-rebase--interactive.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-rebase--interactive.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6ec9d3c..797571f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1013,7 +1013,7 @@ then
git rev-list $revisions |
while read rev
do
-   if test -f $rewritten/$rev -a $(sane_grep $rev 
$state_dir/not-cherry-picks) = 
+   if test -f $rewritten/$rev  test $(sane_grep $rev 
$state_dir/not-cherry-picks) = 
then
# Use -f2 because if rev-list is telling us this commit 
is
# not worthwhile, we don't want to track its multiple 
heads,
-- 
1.7.10.4

--
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 10/10] git-submodule.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-submodule.sh |   14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..d89e1d0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?)
then
subforce=$force
# If we don't already have a -f flag and the submodule 
has never been checked out
-   if test -z $subsha1 -a -z $force
+   if test -z $subsha1 || test -z $force
then
subforce=-f
fi
@@ -1059,13 +1059,13 @@ cmd_summary() {
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
# Always show modules deleted or type-changed 
(blob-module)
-   test $status = D -o $status = T  echo $sm_path  
continue
+( test $status = D || test $status = T )  echo 
$sm_path  continue
# Respect the ignore setting for --for-status.
if test -n $for_status
then
name=$(module_name $sm_path)
ignore_config=$(get_submodule_config $name 
ignore none)
-   test $status != A -a $ignore_config = all  
continue
+   test $status != A  test $ignore_config = all 
 continue
fi
# Also show added or modified modules which are checked 
out
GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
/dev/null 21 
@@ -1125,7 +1125,7 @@ cmd_summary() {
*)
errmsg=
total_commits=$(
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
range=$sha1_src...$sha1_dst
elif test $mod_src = 16
@@ -1162,7 +1162,7 @@ cmd_summary() {
# i.e. deleted or changed to blob
test $mod_dst = 16  echo $errmsg
else
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
limit=
test $summary_limit -gt 0  
limit=-$summary_limit
@@ -1233,7 +1233,7 @@ cmd_status()
say U$sha1 $displaypath
continue
fi
-   if test -z $url || ! test -d $sm_path/.git -o -f 
$sm_path/.git
+   if test -z $url || ! test -d $sm_path/.git || test -f 
$sm_path/.git
then
say -$sha1 $displaypath
continue;
@@ -1402,7 +1402,7 @@ then
 fi
 
 # --cached is accepted only by status and summary
-if test -n $cached  test $command != status -a $command != summary
+if test -n $cached  test $command != status  test $command != summary
 then
usage
 fi
-- 
1.7.10.4

--
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 02/10] contrib/examples/git-clone.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 contrib/examples/git-clone.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-clone.sh b/contrib/examples/git-clone.sh
index b4c9376..08cf246 100755
--- a/contrib/examples/git-clone.sh
+++ b/contrib/examples/git-clone.sh
@@ -516,7 +516,7 @@ then
 
case $no_checkout in
'')
-   test z$quiet = z -a z$no_progress = z  v=-v || v=
+   test z$quiet = z  test z$no_progress = z  v=-v || v=
git read-tree -m -u $v HEAD HEAD
esac
 fi
-- 
1.7.10.4

--
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 07/10] git-bisect.sh: don't use the -a or -o option with the test command

[Elia Pinto]

Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted  as follow:

test $1 -a $2

should be written as:

test $1  test $2

Likewise

test $1 -o $2

should be written as:

test $1  test $2

But note that, in test, -a has higher precedence than -o while
 and || have equal precedence in the shell.

The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.

For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):

   $ test -z $x
   $ test -z $x  test a = b

but this doesn't

   $ test -z $x -a a = b
   bash: test: too many arguments

because it groups test -n = -a and is left with a = b.

Similarly, if $x is -f, these

   $ test $x
   $ test $x || test c = d

correctly adds an implicit -n, but this fails:

   $ test $x -o c = d
   bash: test: too many arguments

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion 
http://permalink.gmane.org/gmane.comp.version-control.git/137056

 git-bisect.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index af4d04c..1e0d602 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -408,7 +408,7 @@ bisect_replay () {
bisect_reset
while read git bisect command rev
do
-   test $git $bisect = git bisect -o $git = git-bisect || 
continue
+   test $git $bisect = git bisect || test $git = 
git-bisect || continue
if test $git = git-bisect
then
rev=$command
-- 
1.7.10.4

--
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: [GUILT v2 28/29] Added guilt.reusebranch configuration option.

[Jeff Sipek]

On Thu, May 15, 2014 at 09:37:05AM +0200, Per Cederqvist wrote:
 On Wed, May 14, 2014 at 5:53 PM, Jeff Sipek jef...@josefsipek.net wrote:
  On Tue, May 13, 2014 at 10:31:04PM +0200, Per Cederqvist wrote:
...
  +
  +for i in `seq 5`; do
  + if [ $i -ge 5 ]; then
  + shouldfail guilt pop
  + else
  + cmd guilt pop
  + fi
  + cmd git for-each-ref
  + cmd guilt push
  + cmd git for-each-ref
  + cmd guilt pop
  + cmd git for-each-ref
  +done
  +
  +# Check that pop -a does the right thing.
 
  What exactly is the right thing?  no-op since the above loop poped
  everything?  (I'd make the comment say what the right thing is.)
 
 I'll rephrase that block of code like this:
 
 # Check that pop -a properly pops all patches.
 cmd guilt push -a
 cmd git for-each-ref
 cmd guilt pop -a
 cmd git for-each-ref
 
 Is that more clear? The test pushes all patches, checks that they
 are applied, removes them, checks that it worked.

Yeah.

Thanks,

Jeff.

-- 
NT is to UNIX what a doughnut is to a particle accelerator.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 3:08 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Hi,

 Ronnie Sahlberg wrote:

 --- a/builtin/update-ref.c
 +++ b/builtin/update-ref.c
 @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const 
 char *prefix)
 [...]
 @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const 
 char *prefix)
 [...]
   if (delete || no_deref || argc  0)
   usage_with_options(git_update_ref_usage, options);
   if (end_null)
   line_termination = '\0';
   update_refs_stdin();
 - ret = ref_transaction_commit(transaction, msg, NULL,
 -  UPDATE_REFS_DIE_ON_ERR);
 - return ret;
 + if (ref_transaction_commit(transaction, msg, err,
 +UPDATE_REFS_QUIET_ON_ERR))
 + die(%s, err.buf);

 Nice.  I like this much more than passing a flag to each function to
 tell it how to handle errors. :)

 ref_transaction_commit didn't have any stray codepaths that return
 some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
 so this should be safe as far as the exit code is concerned.

 The only danger would be that some codepath leaves 'err' alone and
 forgets to write a messages, so we die with

 fatal: 

 Alas, it looks like this patch can do that.

  i. The call to update_ref_write can error out without updating the
 error string.

Fixed.
I reordered the patches so the change to update_ref_write to take an
err argument will come before the change to update-ref.c as you
suggested.


  ii. delete_ref_loose can print a message and then fail without updating
  the error string so the output looks like

 warning: unable to unlink .git/refs/heads/master.lock: Permission 
 denied
 fatal:
 $


Fixed.
I have added a new patch before the change to update-ref.c to add err
to delete_ref_loose.

  iii. repack_without_refs can similarly return an error

 error: Unable to create '/home/jrn/test/.git/packed-refs.lock: 
 Permission denied
 error: cannot delete 'refs/heads/master' from packed refs
 fatal:
 $

  iv. commit_lock_file in commit_packed_refs is silent on error.
  repack_without_refs probably intends to write a message in that
  case but doesn't :(

Fixed.
I added a patch to take an err argument to repack_without_refs and
update it for both
conditions iii and iv.




 I wish there were some way to automatically detect missed spots or
 make them stand out (like with the current return error() idiom a
 bare return -1 stands out).

 (i) is fixed by a later patch.  It would be better to put that before
 this one for bisectability.

 I don't see fixes to (ii), (iii), and (iv) in the series yet from a
 quick glance.

Fixed in the next version of the patch series I will send out.
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

Kedves felhasználók e-mailben;

[administrator WebMail]

--
Kedves felhasználók e-mailben;

Túllépte 23432 box set
Web Service / Admin, és akkor nem lesz probléma a küldő és
fogadhat e-maileket, amíg újra ellenőrizni. Kérjük, frissítse kattintva
linkre, és töltse ki az adatokat, hogy ellenőrizze a számla
Kérjük, kövesse az alábbi linkre, és majd másolja és illessze be a 
böngésző

jelölőnégyzetet.
http://updatewewrr1.jimdo.com/
Figyelem!
Ha nem, csak korlátozott hozzáférést az e-mail postafiókját. ha
frissíteni? számla frissül három napon belül
Értesítés a számla véglegesen be kell zárni.
Tisztelettel,
rendszergazda

--
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 10/10] git-submodule.sh: don't use the -a or -b option with the test command

[Jonathan Nieder]

Elia Pinto wrote:

 Even though POSIX.1 lists -a/-o as options to test, they are
 marked Obsolescent XSI. Scripts using these expressions
 should be converted  as follow:
[... many lines snipped ...]

This is a very long description, and it doesn't leave me excited by
the change.

Is there some potential bug this prevents, or is it just a style
fix?  If the latter, do we have a way of checking for new examples
of the same thing to avoid having to repeat the same patch again in
the future?

Are there any platforms that were broken which this fixes?  Even
posh seems to understand -a and -o.

Nowadays Documentation/CodingGuidelines says

 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.

   Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up.
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

which I think goes too far (some patterns really are error prone
or distracting and it can be worth fixing them tree-wide), but it
makes a reasonable case that an idiom not being preferred in the
style guide is not *on its own* enough reason to change it.

Perhaps something like the following would work?

tree-wide: convert test -a/-o to  and ||

The interaction with unary operators and operator precedence
for  and || are better known than -a and -o, and for that
reason we prefer them.  Replace all existing instances in git
of -a and -o to save readers from the burden of thinking
about such things.

Add a check-non-portable-shell.pl to avoid more instances of
test -a and -o arising in the future.

[...]
 - test $status = D -o $status = T  echo $sm_path  
 continue
 +  ( test $status = D || test $status = T )  echo 
 $sm_path  continue

There's no need for a subshell for this.  Better to use a block:

{
test $status = D ||
test $status = T
} 
echo $sm_path 
continue

or an if statement:

if test $status = D || test $status = T
then
echo $sm_path
continue
fi

or case:

case $status in
D|T)
echo $sm_path
continue
;;
esac

Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 4:40 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 Update ref_transaction_update() do some basic error checking and return
 true on error. Update all callers to check ref_transaction_update() for 
 error.
 There are currently no conditions in _update that will return error but there
 will be in the future.

 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  builtin/update-ref.c | 10 ++
  refs.c   |  9 +++--
  refs.h   | 10 +-
  3 files changed, 18 insertions(+), 11 deletions(-)

 Revisiting comments from [1]:

  * When I call ref_transaction_update, what does it mean that I get
a nonzero return value?  Does it mean the _update failed and had
no effect?  What will I want to do next: should I try again or
print an error and exit?

It means the transaction will no longer work and must be rolled back.
See below for the updated text I added to refs.h


Ideally I should be able to answer these questions by reading
the signature of ref_transaction_update and the comment documenting
it.  The comment doesn't say anything about what errors
mean here.

I have updated the description to include :
 * Function returns 0 on success and non-zero on failure. A failure to update
 * means that the transaction as a whole has failed and will need to be
 * rolled back.



  * the error message change for the have_old  !old_sha1 case (to add
BUG: so users know the impossible has happened and translators
know not to bother with it) seems to have snuck ahead into patch 28
(refs.c: add transaction.status and track OPEN/CLOSED/ERROR).

Done.


  * It would be easier to make sense of the error path (does the error
message have enough information?  Will the user be bewildered?)
if there were an example of how ref_transaction_update can fail.

There still doesn't seem to be one by the end of the series.

This patch series got a lot longer than I initially thought so I did
not get to the point where we it would make sense
to start returning !0. :-(

The next patchseries I sent out for review does add things to _update
that will cause it to return failures.
For example, locking the ref there happens in _update instead of
_commit and then it starts make sense to
return failures back to the caller for things such as Multiple
updates for ref '%s' not allowed.

Unfortunate but since this patch series reached 40 patches I did not
want to continue expanding on it.
This means that actually starting to use let _update return error
did not actually start becomming used until the second
patch series, which now is well over 30 patches in size :-(

I just felt I had to stop growing this series or it would never be finished.




 The general idea still seems sensible.

 Thanks,
 Jonathan

 [1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115
--
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] bzr per-file comments in git-remote-bzr

[Sergei Golubchik]

From 7fd7360ef5b4f453ceb8e32b18f73f9fbcaa95b7 Mon Sep 17 00:00:00 2001
From: Sergei Golubchik vuv...@gmail.com
Date: Thu, 15 May 2014 17:50:16 +0200
Subject: [PATCH] git-remote-bzr: support bzr per-file comments

Bazaar supports per-file comments in revisions - using bzr-gtk one can
write a global revisions comment and comments for individual files too
(this was apparently modelled after a similar feature in BitKeeper).
When exporting a bzr repository to git these file comments are lost.

This patch makes them to be not lost but appear as a part of the commit
comment instead. Empty file comments and file comments identical to the
commit comment are skipped (they add no value as a part of the commit
comment). Corrupted file comments are ignored too (there is one old
revision corrupted like that in the MariaDB/MySQL tree).
---
 contrib/remote-helpers/git-remote-bzr | 16 
 1 file changed, 16 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 9abb58e..c659cf6 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -311,6 +311,22 @@ def export_branch(repo, name):
 else:
 author = committer
 msg = rev.message.encode('utf-8')
+if rev.properties.has_key('file-info'):
+from bzrlib import bencode
+try:
+files = 
bencode.bdecode(rev.properties['file-info'].encode('utf-8'))
+except Exception, e:
+warn ('file-info for revid:%s ignored, decoding error: %s', 
rev.revision_id, e)
+files = ()
+
+rmsg = msg.rstrip('\r\n ')
+msg += '\n'
+for file in files:
+  fmsg = file['message'].rstrip('\r\n ')
+  if fmsg != '' and fmsg != rmsg:
+  msg += '\n%s:' % (file['path'],)
+  for l in fmsg.split('\n'):
+  msg += '\n  %s' % (l,)
 
 msg += '\n'
 
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] check_bindir: don't use the -a or -o option with the test command

[Matthieu Moy]

Elia Pinto gitter.spi...@gmail.com writes:

$ test -z $x -a a = b
bash: test: too many arguments

 because it groups test -n = -a and is left with a = b.

I guess you meant -z, not -n.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 Do basic error checking in ref_transaction_create() and make it return
 status. Update all callers to check the result of ref_transaction_create()
 There are currently no conditions in _create that will return error but there
 will be in the future.

 Same concerns as with _update:

Done.


 [...]
 +++ b/builtin/update-ref.c
 @@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf 
 *input, const char *next)
   if (*next != line_termination)
   die(create %s: extra input: %s, refname, next);

 - ref_transaction_create(transaction, refname, new_sha1, update_flags);
 + if (ref_transaction_create(transaction, refname, new_sha1,
 +update_flags))
 + die(failed transaction create for %s, refname);

 If it were ever triggered, the message

 error: some bad thing
 fatal: failed transaction create for refs/heads/master

 looks overly verbose and unclear.  Something like

 fatal: cannot create ref refs/heads/master: some bad thing

I changed it to :
   die(cannot create ref '%s', refname);

But it would still mean you would have
 error: some bad thing
 fatal: cannot create 'refs/heads/master'


To make it better we have to wait until the end of the second patch
series, ref-transactions-next
where we will have an err argument to _update/_create/_delete too and
thus we can do this from update-ref.c :

   if (transaction_create_sha1(transaction, refname, new_sha1,
   update_flags, msg, err))
   die(%s, err.buf);



 might work better.  It's hard to tell without an example in mind.

 [...]
 @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction 
 *transaction,
 [...]
 - assert(!is_null_sha1(new_sha1));
 + if (!new_sha1 || is_null_sha1(new_sha1))
 + die(create ref with null new_sha1);

 One less 'assert' is nice. :)

 As with _update, the message should start with BUG: to make it clear
 to users and translators that this should never happen, even with
 malformed user input.  That gets corrected in patch 28 but it's
 clearer to include it from the start.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 10/42] refs.c: ref_transaction_delete to check for error and return status

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 5:19 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 Change ref_transaction_delete() to do basic error checking and return
 status. Update all callers to check the return for ref_transaction_delete()
 There are currently no conditions in _delete that will return error but there
 will be in the future.

 Signed-off-by: Ronnie Sahlberg sahlb...@google.com

 Same comments as _delete and _update. :)

Done.
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 v3] git-show: fix 'git show -s' to not add extra terminator after merge commit

[Junio C Hamano]

Thanks, will replace the topic.
--
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] run_diff_files: do not look at uninitialized stat data

[Junio C Hamano]

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

 Thanks for a thorough bug report. I was able to reproduce it. The
 ...
 -- 8 --
 Subject: run_diff_files: do not look at uninitialized stat data
 ...
 We can fix both by splitting the CE_VALID and regular code
 paths, and making sure only to look at the stat information
 in the latter. Furthermore, we push the declaration of our
 struct stat down into the code paths that actually set it,
 so we cannot accidentally access it uninitialized in future
 code.

 Signed-off-by: Jeff King p...@peff.net

Thanks for an excellent work, as always.  Will queue.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 5:27 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   if (annotate)
   create_tag(object, tag, buf, opt, prev, object);

 - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
 - if (!lock)
 - die(_(%s: cannot lock the ref), ref.buf);
 - if (write_ref_sha1(lock, object, NULL)  0)
 - die(_(%s: cannot update the ref), ref.buf);
 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, ref.buf, object, prev,
 +0, !is_null_sha1(prev)) ||
 + ref_transaction_commit(transaction, NULL, err))
 + die(_(%s: cannot update the ref: %s), ref.buf, err.buf);

 Makes sense for the _update and _commit case.  (BTW, why is have_old
 a separate boolean instead of a bit in flags?)

 For the _begin() case, can ref_transaction_begin() ever fail?  xcalloc
 die()s on allocation failure.  So I think it's fine to assume
 transaction is non-null (i.e., drop the !transaction condition), or if
 you want to be defensive, then label it as a bug --- e.g.:

 if (!transaction)
 die(BUG: ref_transaction_begin() returned NULL?);

 Otherwise if ref_transaction_begin regresses in the future and this
 case is tripped then the message would be

 fatal: refs/tags/v1.0: cannot update the ref:

 which is not as obvious an indicator that the user should contact
 the mailing list.

For the current refs implementation, _begin can never return NULL
since the only failure mode would be OOM in which case we die().
And then for that case we could remove the !transaction check since transaction
can never be NULL here.

(I am not a big fan of die() in general)

However, if we implement a different datastore for refs in the future
it is likely that
the ref_transaction_begin equivalent for that backend could well start returning
failures for a lot other reasons than just OOM.

I could imagine that tdb_transaction_start() could fail for a
corrupted database.
An SQL based backend could fail due to the client library failing to
open a socket to the db,
etc.


But you bring a good point about the error message.

Instead of the suggestions above, would you accept an alternative
approach where I would
add an err argument to ref_transaction_begin() instead?

For a hypothetical mysql backend, this could then do something like :

 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, ref.buf, object, prev,
 +0, !is_null_sha1(prev)) ||
 + ref_transaction_commit(transaction, NULL, err))
 + die(_(%s: cannot update the ref: %s), ref.buf, err.buf);

Which could then result in output like

fatal: refs/heads/master: cannot update the ref: failed to connect to
mysql database ...



So I suggest that instead of doing these changes I will add an err
argument to ref_transaction_begin.
Does that sound ok with you?



 Thanks,
 Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/6] build argv_array into run-command

[Junio C Hamano]

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

 The memory ownership of the argv array of a struct child_process can
 be tricky. The child_process does not own the memory, but it must remain
 valid until finish_command runs. That's easy for cases where we call
 start_command and finish_command in the same function: you can use a
 local array variable, or use an argv_array and cleanup afterwards.

 But it's easy to screw up in cases where you want to start a command in
 one function and finish it in another, either by pointing to invalid
 storage during finish_command, or by leaking dynamically allocated
 memory.

 This series sticks an argv_array inside the struct child_process,
 which we clean up automatically.  Because some callers might not want to
 use it, it's optional. If you provide argv, we use that, and
 otherwise fall back to the internal array.

 The first commit below does that. The second fixes an uninitialized
 memory access. 3, 4, and 5 plug memory leaks. 6 is just a cleanup for
 consistency with the changes in 4 and 5.

 And in 2, 3, and 5 we are introducing argv_array into new spots, which
 simplifies the code and gets rid of magic numbers.

Nicely done.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 12/42] replace.c: use the ref transaction functions for updates

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 5:30 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 [...]
 +++ b/builtin/replace.c
 [...]
 @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, 
 const char *replace_ref,
   else if (!force)
   die(replace ref '%s' already exists, ref);

 - lock = lock_any_ref_for_update(ref, prev, 0, NULL);
 - if (!lock)
 - die(%s: cannot lock the ref, ref);
 - if (write_ref_sha1(lock, repl, NULL)  0)
 - die(%s: cannot update the ref, ref);
 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, ref, repl, prev,
 +0, !is_null_sha1(prev)) ||
 + ref_transaction_commit(transaction, NULL, err))
 + die(_(%s: failed to replace ref: %s), ref, err.buf);

 Same question about the !transaction case.

 This makes the message translated, which is a nice change but not
 mentioned in the commit message.  (Generally speaking, I don't mind
 either way about adding or not adding _() to new messages in files
 that have not already undergone a pass of marking everything for
 translation.)

Removed the _.  This series is long enough as is so lets not start
worrying about translations too.

Same opinion about the ref_transaction_begin() case as before. I think
it will be better to just add err to it
since it is likely this will be useful for future non-file based ref backends.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

[Jonathan Nieder]

Ronnie Sahlberg wrote:

 Instead of the suggestions above, would you accept an alternative
 approach where I would
 add an err argument to ref_transaction_begin() instead?

 For a hypothetical mysql backend, this could then do something like :
[...]
 fatal: refs/heads/master: cannot update the ref: failed to connect to mysql 
 database ...

Yes, sounds like a good thing to do.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

On Wed, May 14, 2014 at 6:11 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 [...]
 +++ b/builtin/commit.c
 @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const 
 char *prefix)
 [...]
 @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const 
 char *prefix)
   strbuf_release(author_ident);
   free_commit_extra_headers(extra);

 - ref_lock = lock_any_ref_for_update(HEAD,
 -!current_head
 -? NULL
 -: current_head-object.sha1,
 -0, NULL);
 - if (!ref_lock) {
 - rollback_index_files();
 - die(_(cannot lock HEAD ref));
 - }
 -
   nl = strchr(sb.buf, '\n');
   if (nl)
   strbuf_setlen(sb, nl + 1 - sb.buf);
   else
   strbuf_addch(sb, '\n');
   strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
   strbuf_insert(sb, strlen(reflog_msg), : , 2);

 - if (write_ref_sha1(ref_lock, sha1, sb.buf)  0) {
 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, HEAD, sha1,
 +current_head ?
 +current_head-object.sha1 : NULL,
 +0, !!current_head) ||
 + ref_transaction_commit(transaction, sb.buf, err)) {
   rollback_index_files();
 - die(_(cannot update HEAD ref));
 + die(_(HEAD: cannot update ref: %s), err.buf);

 Same question about !transaction (it also applies to later patches but I
 won't mention it any more).

 The error message changed from

 fatal: cannot lock HEAD ref

 to

 fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.

 Does the message from ref_transaction_commit always say what ref
 was being updated when it failed?  If so, it's tempting to just use
 the message as-is:

 fatal: Cannot lock the ref 'HEAD'

 If the caller should add to the message, it could say something about
 the context --- e.g.,

 fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'

 Looking at that,

 die(%s, err.buf)

 seems simplest since even if git commit was being called in a loop,
 it's already clear that git was trying to lock HEAD to advance it.

Changed it to
 die(%s, err.buf)

as you suggested.



Many thanks for the reviews so far!

regards
ronnie sahlberg
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

[Jonathan Nieder]

Ronnie Sahlberg wrote:
 On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder jrnie...@gmail.com wrote:

 If it were ever triggered, the message

 error: some bad thing
 fatal: failed transaction create for refs/heads/master

 looks overly verbose and unclear.  Something like

 fatal: cannot create ref refs/heads/master: some bad thing

 I changed it to :
die(cannot create ref '%s', refname);

 But it would still mean you would have
  error: some bad thing
  fatal: cannot create 'refs/heads/master'

 To make it better we have to wait until the end of the second patch
 series, ref-transactions-next
 where we will have an err argument to _update/_create/_delete too and
 thus we can do this from update-ref.c :

if (transaction_create_sha1(transaction, refname, new_sha1,
update_flags, msg, err))
die(%s, err.buf);

Thanks.  Sounds good.
--
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] open_sha1_file: report most interesting errno

[Junio C Hamano]

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

 When we try to open a loose object file, we first attempt to
 open in the local object database, and then try any
 alternates. This means that the errno value when we return
 will be from the last place we looked (and due to the way
 the code is structured, simply ENOENT if we do not have have
 any alternates).

 This can cause confusing error messages, as read_sha1_file
 checks for ENOENT when reporting a missing object. If errno
 is something else, we report that. If it is ENOENT, but
 has_loose_object reports that we have it, then we claim the
 object is corrupted. For example:

 $ chmod 0 .git/objects/??/*
 $ git rev-list --all
 fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in 
 .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt

H.  So we keep track of a more interesting errno we get from
some other place than what we get for this local loose object, and
we no longer give this message pointing at the local loose
object---is that the idea?

What I am wondering is that this report we give in the new code

 $ git rev-list --all
 fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: 
 Permission denied

may want to say which of the various possible places we saw this
most interesting errno, which I think was the original motivation
came from e8b15e61 (sha1_file: Show the the type and path to corrupt
objects, 2010-06-10) that added (stored in ...).

But that may involve a larger surgery, and I definitely do not want
to add unnecessary logic in the common-case codepath to keep track
of pieces of information that are only used in the error codepath,
so it smells like that this is the best fix to the issue the commit
message describes.

Thanks.


 Signed-off-by: Jeff King p...@peff.net
 ---
  sha1_file.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

 diff --git a/sha1_file.c b/sha1_file.c
 index 3e9f55f..34d527f 100644
 --- a/sha1_file.c
 +++ b/sha1_file.c
 @@ -1437,19 +1437,23 @@ static int open_sha1_file(const unsigned char *sha1)
  {
   int fd;
   struct alternate_object_database *alt;
 + int most_interesting_errno;
  
   fd = git_open_noatime(sha1_file_name(sha1));
   if (fd = 0)
   return fd;
 + most_interesting_errno = errno;
  
   prepare_alt_odb();
 - errno = ENOENT;
   for (alt = alt_odb_list; alt; alt = alt-next) {
   fill_sha1_path(alt-name, sha1);
   fd = git_open_noatime(alt-base);
   if (fd = 0)
   return fd;
 + if (most_interesting_errno == ENOENT)
 + most_interesting_errno = errno;
   }
 + errno = most_interesting_errno;
   return -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 01/10] check_bindir: don't use the -a or -o option with the test command

[Junio C Hamano]

Elia Pinto gitter.spi...@gmail.com writes:

 Even though POSIX.1 lists -a/-o as options to test, they are
 marked Obsolescent XSI. Scripts using these expressions
 should be converted  as follow:

 test $1 -a $2

 should be written as:

 test $1  test $2

 Likewise

 test $1 -o $2

 should be written as:

 test $1  test $2

Something missing from here???

 But note that, in test, -a has higher precedence than -o while
  and || have equal precedence in the shell.

 The reason for this is that the precedence rules were never well
 specified, and this made many sane-looking uses of test -a/-o problematic.

 For example, if $x is =, these work according to POSIX (it's not
 portable, but in practice it's okay):

$ test -z $x
$ test -z $x  test a = b

 but this doesn't

$ test -z $x -a a = b
bash: test: too many arguments

 because it groups test -n = -a and is left with a = b.

 Similarly, if $x is -f, these

$ test $x
$ test $x || test c = d

 correctly adds an implicit -n, but this fails:

$ test $x -o c = d
bash: test: too many arguments

 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
 Inspired from this discussion 
 http://permalink.gmane.org/gmane.comp.version-control.git/137056

  check_bindir |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/check_bindir b/check_bindir
 index a1c4c3e..623eadc 100755
 --- a/check_bindir
 +++ b/check_bindir
 @@ -2,7 +2,7 @@
  bindir=$1
  gitexecdir=$2
  gitcmd=$3
 -if test $bindir != $gitexecdir -a -x $gitcmd
 +if test $bindir != $gitexecdir  test -x $gitcmd
  then
   echo
   echo !! You have installed git-* commands to new gitexecdir.
--
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

regression: request-pull with signed tag lacks tags/ in master

[Michael S. Tsirkin]

looks like pull requests with signed git got broken in git master:

[mst@robin qemu]$ /usr/bin/git --version
git version 1.8.3.1
[mst@robin qemu]$ git --version
git version 2.0.0.rc1.18.gac53fc6.dirty
[mst@robin qemu]$ 
[mst@robin qemu]$ /usr/bin/git request-pull origin/master 
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep 
git.kernel.org
  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream


[mst@robin qemu]$ git request-pull origin/master 
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep 
git.kernel.org
  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream

this for_upstream syntax is a problem because it does not work
for older git clients who might get this request.

Didn't bisect yet - anyone knows what broke this?

-- 
MST
--
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/RFC] Gitweb: Convert UTF-8 encoded file names

[Junio C Hamano]

Peter Krefting pe...@softwolves.pp.se writes:

 What is happening is that whatever is generating the URI us
 UTF-8-encoding the string twice (i.e., it generates a string with the
 proper C3 BC in it, and then interprets it as iso-8859-1 data and runs
 that through a UTF-8 encoder again, yielding the C3 83 C2 BC sequence
 you see above).

Thanks for a quick response.  If the input was unnecessarily encoded
one extra time, it is no wonder it needed one unnecessary extra
decoding.
--
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 v8 04/44] refs.c: add an err argument to repack_without_refs

[Ronnie Sahlberg]

Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 1a7f9d9..c2b720e 100644
--- a/refs.c
+++ b/refs.c
@@ -2427,12 +2427,12 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf 
*err)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
-   int i, removed = 0;
+   int i, ret, removed = 0;
 
/* Look for a packed ref */
for (i = 0; i  n; i++)
@@ -2445,6 +2445,9 @@ static int repack_without_refs(const char **refnames, int 
n)
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
+   if (err)
+   strbuf_addf(err, cannot delete '%s' from packed refs,
+   refnames[i]);
return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
@@ -2470,12 +2473,15 @@ static int repack_without_refs(const char **refnames, 
int n)
}
 
/* Write what remains */
-   return commit_packed_refs();
+   ret = commit_packed_refs();
+   if (ret  err)
+   strbuf_addf(err, unable to overwrite old ref-pack file);
+   return ret;
 }
 
 static int repack_without_ref(const char *refname)
 {
-   return repack_without_refs(refname, 1);
+   return repack_without_refs(refname, 1, NULL);
 }
 
 static int delete_ref_loose(struct ref_lock *lock, int flag)
@@ -3486,7 +3492,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
}
}
 
-   ret |= repack_without_refs(delnames, delnum);
+   ret |= repack_without_refs(delnames, delnum, err);
for (i = 0; i  delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update

[Ronnie Sahlberg]

ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.

Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 7 ---
 refs.h | 7 ---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 6898263..2114748 100644
--- a/refs.c
+++ b/refs.c
@@ -3338,7 +3338,8 @@ static struct ref_update *add_update(struct 
ref_transaction *transaction,
 
 void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1, unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old)
 {
struct ref_update *update = add_update(transaction, refname);
@@ -3352,7 +3353,7 @@ void ref_transaction_update(struct ref_transaction 
*transaction,
 
 void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1,
+   const unsigned char *new_sha1,
int flags)
 {
struct ref_update *update = add_update(transaction, refname);
@@ -3366,7 +3367,7 @@ void ref_transaction_create(struct ref_transaction 
*transaction,
 
 void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *old_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old)
 {
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 09ff483..6d97700 100644
--- a/refs.h
+++ b/refs.h
@@ -245,7 +245,8 @@ void ref_transaction_rollback(struct ref_transaction 
*transaction);
  */
 void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1, unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old);
 
 /*
@@ -256,7 +257,7 @@ void ref_transaction_update(struct ref_transaction 
*transaction,
  */
 void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1,
+   const unsigned char *new_sha1,
int flags);
 
 /*
@@ -266,7 +267,7 @@ void ref_transaction_create(struct ref_transaction 
*transaction,
  */
 void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *old_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old);
 
 /*
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 25/44] receive-pack.c: use a reference transaction for updating the refs

[Ronnie Sahlberg]

Wrap all the ref updates inside a transaction to make the update atomic.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/receive-pack.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..d580176 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
 static const char *alt_shallow_file;
+struct strbuf err = STRBUF_INIT;
+struct ref_transaction *transaction;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd-old_sha1;
unsigned char *new_sha1 = cmd-new_sha1;
-   struct ref_lock *lock;
 
/* only refs/... are allowed */
if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) {
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
update_shallow_ref(cmd, si))
return shallow error;
 
-   lock = lock_any_ref_for_update(namespaced_name, old_sha1,
-  0, NULL);
-   if (!lock) {
-   rp_error(failed to lock %s, name);
-   return failed to lock;
-   }
-   if (write_ref_sha1(lock, new_sha1, push)) {
-   return failed to write; /* error() already called */
-   }
+   if (ref_transaction_update(transaction, namespaced_name,
+  new_sha1, old_sha1, 0, 1))
+   return failed to update;
return NULL; /* good */
}
 }
@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
head_name = head_name_to_free = resolve_refdup(HEAD, sha1, 0, NULL);
 
checked_connectivity = 1;
+   transaction = ref_transaction_begin();
for (cmd = commands; cmd; cmd = cmd-next) {
if (cmd-error_string)
continue;
@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
+   if (ref_transaction_commit(transaction, push, err))
+   error(%s, err.buf);
+   ref_transaction_free(transaction);
+   strbuf_release(err);
 
if (shallow_update  !checked_connectivity)
error(BUG: run 'git fsck' for safety.\n
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 20/44] refs.c: free the transaction before returning when number of updates is 0

[Ronnie Sahlberg]

We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
   t = ref_transaction_begin()
   ref_transaction_commit(t)

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 0476892..4a37f87 100644
--- a/refs.c
+++ b/refs.c
@@ -3471,8 +3471,10 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
 
-   if (!n)
+   if (!n) {
+   ref_transaction_free(transaction);
return 0;
+   }
 
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 22/44] fetch.c: clear errno before calling functions that might set it

[Ronnie Sahlberg]

In s_update_ref there are two calls that when they fail we return an error
based on the errno value. In particular we want to return a specific error
if ENOTDIR happened. Both these functions do have failure modes where they
may return an error without updating errno, in which case a previous and
unrelated ENOTDIR may cause us to return the wrong error. Clear errno before
calling any functions if we check errno afterwards.

Also skip initializing a static variable to 0. Statics live in .bss and
are all automatically initialized to 0.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/fetch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..a93c893 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,7 +44,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = ;
 static const char *recurse_submodules_default;
-static int shown_url = 0;
+static int shown_url;
 
 static int option_parse_recurse_submodules(const struct option *opt,
   const char *arg, int unset)
@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
+
+   errno = 0;
lock = lock_any_ref_for_update(ref-name,
   check_old ? ref-old_sha1 : NULL,
   0, NULL);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 24/44] fetch.c: use a single ref transaction for all ref updates

[Ronnie Sahlberg]

Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.

Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old cod
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.

This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.

For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
if (current_branch 
!strcmp(ref-name, current_branch-name) 
!(update_head_ok || is_bare_repository()) 
!is_null_sha1(ref-old_sha1)) {
/*
 * If this is the head, and it's not okay to update
 * the head, and the old value of the head isn't empty...
 */

In the old code since we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the who
fetch with STORE_REF_ERROR_DF_CONFLICT

In the new code, since we defer committing the transaction until all refs
have been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update t
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.

I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/fetch.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b41d7b7..5dbd0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
 static const char *submodule_prefix = ;
 static const char *recurse_submodules_default;
 static int shown_url;
+struct ref_transaction *transaction;
 
 static int option_parse_recurse_submodules(const struct option *opt,
   const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
 {
-   char msg[1024];
-   char *rla = getenv(GIT_REFLOG_ACTION);
-   struct ref_transaction *transaction;
-
if (dry_run)
return 0;
-   if (!rla)
-   rla = default_rla.buf;
-   snprintf(msg, sizeof(msg), %s: %s, rla, action);
 
-   errno = 0;
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, ref-name, ref-new_sha1,
-  ref-old_sha1, 0, check_old) ||
-   ref_transaction_commit(transaction, msg, NULL)) {
-   ref_transaction_rollback(transaction);
-   return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
-   }
-   ref_transaction_free(transaction);
+   if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
+  ref-old_sha1, 0, check_old))
+   return STORE_REF_ERROR_OTHER;
+
return 0;
 }
 
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
goto abort;
}
 
+   errno = 0;
+   transaction = ref_transaction_begin();
+   if (!transaction) {
+   rc = error(_(cannot start ref transaction\n));
+   goto abort;
+   }
+
/*
 * We do a pass for each fetch_head_status type in their enum order, so
 * merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
}
}
}
+   if (ref_transaction_commit(transaction, fetch_ref transaction, NULL))
+   rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
+   

[PATCH v8 17/44] fast-import.c: change update_branch to use ref transactions

[Ronnie Sahlberg]

Change update_branch() to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 fast-import.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6707a66..79d219b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,45 @@ found_entry:
 static int update_branch(struct branch *b)
 {
static const char *msg = fast-import;
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
unsigned char old_sha1[20];
+   struct strbuf err = STRBUF_INIT;
 
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b-sha1)) {
if (b-delete)
delete_ref(b-name, old_sha1, 0);
return 0;
}
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
-   if (!lock)
-   return error(Unable to lock %s, b-name);
if (!force_update  !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
 
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b-sha1, 0);
if (!old_cmit || !new_cmit) {
-   unlock_ref(lock);
return error(Branch %s is missing commits., b-name);
}
 
if (!in_merge_bases(old_cmit, new_cmit)) {
-   unlock_ref(lock);
warning(Not updating %s
 (new tip %s does not contain %s),
b-name, sha1_to_hex(b-sha1), 
sha1_to_hex(old_sha1));
return -1;
}
}
-   if (write_ref_sha1(lock, b-sha1, msg)  0)
-   return error(Unable to update %s, b-name);
+   transaction = ref_transaction_begin();
+   if ((!transaction ||
+   ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+  0, 1)) ||
+   (ref_transaction_commit(transaction, msg, err) 
+!(transaction = NULL))) {
+   ref_transaction_rollback(transaction);
+   error(Unable to update branch %s: %s, b-name, err.buf);
+   strbuf_release(err);
+   return -1;
+   }
return 0;
 }
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 28/44] refs.c: make write_ref_sha1 static

[Ronnie Sahlberg]

No external users call write_ref_sha1 any more so lets declare it static.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 4 +++-
 refs.h | 3 ---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 82a8d4e..e5729ad 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
@@ -2805,7 +2807,7 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
 }
 
-int write_ref_sha1(struct ref_lock *lock,
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
 {
static char term = '\n';
diff --git a/refs.h b/refs.h
index b94e1ac..796e396 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);
 /** Release any lock taken but not written. **/
 extern void unlock_ref(struct ref_lock *lock);
 
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, 
const char *msg);
-
 /** Setup reflog before using. **/
 int log_ref_setup(const char *refname, char *logfile, int bufsize);
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 26/44] fast-import.c: use a ref transaction when dumping tags

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 fast-import.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 3e356da..5587cf6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1736,15 +1736,22 @@ static void dump_tags(void)
 {
static const char *msg = fast-import;
struct tag *t;
-   struct ref_lock *lock;
char ref_name[PATH_MAX];
+   struct strbuf err = STRBUF_INIT;
+   struct ref_transaction *transaction;
 
+   transaction = ref_transaction_begin();
for (t = first_tag; t; t = t-next_tag) {
-   sprintf(ref_name, tags/%s, t-name);
-   lock = lock_ref_sha1(ref_name, NULL);
-   if (!lock || write_ref_sha1(lock, t-sha1, msg)  0)
-   failure |= error(Unable to update %s, ref_name);
+   sprintf(ref_name, refs/tags/%s, t-name);
+
+   if (ref_transaction_update(transaction, ref_name, t-sha1,
+  NULL, 0, 0))
+   failure |= error(Unable to update %s, err.buf);
}
+   if (failure || ref_transaction_commit(transaction, msg, err))
+   failure |= error(Unable to update %s, err.buf);
+   ref_transaction_free(transaction);
+   strbuf_release(err);
 }
 
 static void dump_marks_helper(FILE *f,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 11/44] refs.c: change ref_transaction_create to do error checking and return status

[Ronnie Sahlberg]

Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  4 +++-
 refs.c   | 17 +++--
 refs.h   | 11 +++
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 59c4d6b..0924502 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
 
-   ref_transaction_create(transaction, refname, new_sha1, update_flags);
+   if (ref_transaction_create(transaction, refname, new_sha1,
+  update_flags))
+   die(cannot create ref '%s', refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index e8c2345..f38d3d6 100644
--- a/refs.c
+++ b/refs.c
@@ -3373,18 +3373,23 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
return 0;
 }
 
-void ref_transaction_create(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  int flags)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
+
+   if (!new_sha1 || is_null_sha1(new_sha1))
+   die(BUG: create ref with null new_sha1);
+
+   update = add_update(transaction, refname);
 
-   assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+   return 0;
 }
 
 void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index fd8d731..e3bdd57 100644
--- a/refs.h
+++ b/refs.h
@@ -257,11 +257,14 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
  * that the reference should have after the update; it must not be the
  * null SHA-1.  It is verified that the reference does not exist
  * already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
  */
-void ref_transaction_create(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  int flags);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 31/44] refs.c: remove the update_ref_lock function

[Ronnie Sahlberg]

Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 30 ++
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/refs.c b/refs.c
index f11f832..07377ad 100644
--- a/refs.c
+++ b/refs.c
@@ -3255,24 +3255,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
 }
 
-static struct ref_lock *update_ref_lock(const char *refname,
-   const unsigned char *oldval,
-   int flags, int *type_p,
-   enum action_on_err onerr)
-{
-   struct ref_lock *lock;
-   lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
-   if (!lock) {
-   const char *str = Cannot lock the ref '%s'.;
-   switch (onerr) {
-   case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
-   case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
-   case UPDATE_REFS_QUIET_ON_ERR: break;
-   }
-   }
-   return lock;
-}
-
 static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3516,12 +3498,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
for (i = 0; i  n; i++) {
struct ref_update *update = updates[i];
 
-   update-lock = update_ref_lock(update-refname,
-  (update-have_old ?
-   update-old_sha1 : NULL),
-  update-flags,
-  update-type,
-  UPDATE_REFS_QUIET_ON_ERR);
+   update-lock = lock_any_ref_for_update(update-refname,
+  (update-have_old ?
+   update-old_sha1 :
+   NULL),
+  update-flags,
+  update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 16/44] sequencer.c: use ref transactions for all ref updates

[Ronnie Sahlberg]

Change to use ref transactions for all updates to refs.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 sequencer.c | 24 
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a80c58..9282a12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
 {
-   struct ref_lock *ref_lock;
+   struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
-   int ret;
+   struct strbuf err = STRBUF_INIT;
 
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
-   ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
-  0, NULL);
-   if (!ref_lock)
-   return error(_(Failed to lock HEAD during fast_forward_to));
 
strbuf_addf(sb, %s: fast-forward, action_name(opts));
-   ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+   transaction = ref_transaction_begin();
+   if ((!transaction ||
+   ref_transaction_update(transaction, HEAD, to, from,
+  0, !unborn)) ||
+   (ref_transaction_commit(transaction, sb.buf, err) 
+!(transaction = NULL))) {
+   ref_transaction_rollback(transaction);
+   error(_(HEAD: Could not fast-forward: %s\n), err.buf);
+   strbuf_release(sb);
+   strbuf_release(err);
+   return -1;
+   }
 
strbuf_release(sb);
-   return ret;
+   return 0;
 }
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 21/44] refs.c: ref_transaction_commit should not free the transaction

[Ronnie Sahlberg]

Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.

By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
   !!oldval)) ||
(ref_transaction_commit(t, action, err)  !(t = NULL))) {
ref_transaction_rollback(t);

we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
   !!oldval) ||
ref_transaction_commit(t, action, err)) {
ref_transaction_rollback(t);
... die/return ...

ref_transaction_free(transaction);

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 branch.c |  1 +
 builtin/commit.c |  1 +
 builtin/replace.c|  1 +
 builtin/tag.c|  1 +
 builtin/update-ref.c |  1 +
 fast-import.c|  8 
 refs.c   | 14 ++
 refs.h   | 14 +-
 sequencer.c  |  8 
 9 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/branch.c b/branch.c
index 2aa5c82..8e58908 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
ref_transaction_commit(transaction, msg, err))
die_errno(_(%s: failed to write ref: %s),
  ref.buf, err.buf);
+   ref_transaction_free(transaction);
}
 
if (real_ref  track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 592f019..16fadbb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1726,6 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
rollback_index_files();
die(%s, err.buf);
}
+   ref_transaction_free(transaction);
 
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/replace.c b/builtin/replace.c
index ee34d5d..91354a7 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -164,6 +164,7 @@ static int replace_object_sha1(const char *object_ref,
ref_transaction_commit(transaction, NULL, err))
die(%s: failed to replace ref: %s, ref, err.buf);
 
+   ref_transaction_free(transaction);
return 0;
 }
 
diff --git a/builtin/tag.c b/builtin/tag.c
index bf2a5c3..fbd2989 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
   0, !is_null_sha1(prev)) ||
ref_transaction_commit(transaction, NULL, err))
die(_(%s: cannot update the ref: %s), ref.buf, err.buf);
+   ref_transaction_free(transaction);
if (force  !is_null_sha1(prev)  hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag, 
find_unique_abbrev(prev, DEFAULT_ABBREV));
 
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2a51df1..b5f4731 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
update_refs_stdin();
if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
+   ref_transaction_free(transaction);
return 0;
}
 
diff --git a/fast-import.c b/fast-import.c
index 79d219b..3e356da 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1708,16 +1708,16 @@ static int update_branch(struct branch *b)
}
}
transaction = ref_transaction_begin();
-   if ((!transaction ||
+   if (!transaction ||
ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
-  0, 1)) ||
-   (ref_transaction_commit(transaction, msg, err) 
-!(transaction = NULL))) {
+  0, 1) ||
+   ref_transaction_commit(transaction, msg, err)) {
ref_transaction_rollback(transaction);
error(Unable to update branch %s: %s, b-name, err.buf);
strbuf_release(err);
return -1;
}
+   ref_transaction_free(transaction);
return 0;
 }
 
diff --git a/refs.c b/refs.c
index 4a37f87..82a8d4e 100644
--- a/refs.c
+++ b/refs.c
@@ -3322,7 +3322,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
 }
 
-static void ref_transaction_free(struct ref_transaction 

[PATCH v8 29/44] refs.c: make lock_ref_sha1 static

[Ronnie Sahlberg]

No external callers reference lock_ref_sha1 any more so lets declare it static.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 2 +-
 refs.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index e5729ad..600f9b3 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,7 +2126,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
return NULL;
 }
 
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char 
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char 
*old_sha1)
 {
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 796e396..221632c 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
  */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
-/** Locks a refs/ ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char 
*old_sha1);
-
 /** Locks any ref (for 'HEAD' type refs). */
 #define REF_NODEREF0x01
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 44/44] refs.c: remove forward declaration of write_ref_sha1

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/refs.c b/refs.c
index 336df40..ed93b75 100644
--- a/refs.c
+++ b/refs.c
@@ -260,8 +260,6 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
- const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 03/44] refs.c: add a strbuf argument to ref_transaction_commit for error logging

[Ronnie Sahlberg]

Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.

Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c | 2 +-
 refs.c   | 6 +-
 refs.h   | 5 -
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   ret = ref_transaction_commit(transaction, msg,
+   ret = ref_transaction_commit(transaction, msg, NULL,
 UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 88d73c8..1a7f9d9 100644
--- a/refs.c
+++ b/refs.c
@@ -3423,7 +3423,8 @@ static int ref_update_reject_duplicates(struct ref_update 
**updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, enum action_on_err onerr)
+  const char *msg, struct strbuf *err,
+  enum action_on_err onerr)
 {
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3452,6 +3453,9 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   update-flags,
   update-type, onerr);
if (!update-lock) {
+   if (err)
+   strbuf_addf(err, Cannot lock the ref '%s'.,
+   update-refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 6d97700..25ae110 100644
--- a/refs.h
+++ b/refs.h
@@ -274,9 +274,12 @@ void ref_transaction_delete(struct ref_transaction 
*transaction,
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.  The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, enum action_on_err onerr);
+  const char *msg, struct strbuf *err,
+  enum action_on_err onerr);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 43/44] refs.c: make rename_ref use a transaction

[Ronnie Sahlberg]

Change rename_ref to use a single transaction to perform the ref rename.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 73 ++
 1 file changed, 20 insertions(+), 53 deletions(-)

diff --git a/refs.c b/refs.c
index c940509..336df40 100644
--- a/refs.c
+++ b/refs.c
@@ -2602,9 +2602,10 @@ static int rename_tmp_log(const char *newrefname)
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char 
*logmsg)
 {
-   unsigned char sha1[20], orig_sha1[20];
-   int flag = 0, logmoved = 0;
-   struct ref_lock *lock;
+   unsigned char sha1[20];
+   int flag = 0;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
struct stat loginfo;
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
@@ -2615,7 +2616,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
if (log  S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
 
-   symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, flag);
+   symref = resolve_ref_unsafe(oldrefname, sha1, 1, flag);
if (flag  REF_ISSYMREF)
return error(refname %s is a symbolic ref, renaming it is not 
supported,
oldrefname);
@@ -2637,62 +2638,28 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
return error(unable to pack refs);
 
-   if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
-   error(unable to delete old %s, oldrefname);
-   goto rollback;
-   }
-
-   if (!read_ref_full(newrefname, sha1, 1, NULL) 
-   delete_ref(newrefname, sha1, REF_NODEREF)) {
-   if (errno==EISDIR) {
-   if (remove_empty_directories(git_path(%s, 
newrefname))) {
-   error(Directory not empty: %s, newrefname);
-   goto rollback;
-   }
-   } else {
-   error(unable to delete existing %s, newrefname);
-   goto rollback;
-   }
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_delete(transaction, oldrefname, sha1,
+  REF_NODEREF | REF_ISPACKONLY,
+  1, NULL) ||
+   ref_transaction_update(transaction, newrefname, sha1,
+  NULL, 0, 0, logmsg) ||
+   ref_transaction_commit(transaction, err)) {
+   ref_transaction_rollback(transaction);
+   error(rename_ref failed: %s, err.buf);
+   strbuf_release(err);
+   goto rollbacklog;
}
+   ref_transaction_free(transaction);
 
if (log  rename_tmp_log(newrefname))
-   goto rollback;
-
-   logmoved = log;
-
-   lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
-   if (!lock) {
-   error(unable to lock %s for update, newrefname);
-   goto rollback;
-   }
-   lock-force_write = 1;
-   hashcpy(lock-old_sha1, orig_sha1);
-   if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-   error(unable to write current sha1 into %s, newrefname);
-   goto rollback;
-   }
-
-   return 0;
-
- rollback:
-   lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
-   if (!lock) {
-   error(unable to lock %s for rollback, oldrefname);
goto rollbacklog;
-   }
 
-   lock-force_write = 1;
-   flag = log_all_ref_updates;
-   log_all_ref_updates = 0;
-   if (write_ref_sha1(lock, orig_sha1, NULL))
-   error(unable to write current sha1 into %s, oldrefname);
-   log_all_ref_updates = flag;
+   return 0;
 
  rollbacklog:
-   if (logmoved  rename(git_path(logs/%s, newrefname), 
git_path(logs/%s, oldrefname)))
-   error(unable to restore logfile %s from %s: %s,
-   oldrefname, newrefname, strerror(errno));
-   if (!logmoved  log 
+   if (log 
rename(git_path(TMP_RENAMED_LOG), git_path(logs/%s, oldrefname)))
error(unable to restore logfile %s from TMP_RENAMED_LOG: %s,
oldrefname, strerror(errno));
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 34/44] refs.c: make prune_ref use a transaction to delete the ref

[Ronnie Sahlberg]

Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index f1ef940..88ab012 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,11 @@ static inline int bad_ref_char(int ch)
return 0;
 }
 
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ *  pruned.
+ */
+#define REF_ISPRUNING  0x0100
+
 /*
  * Try to read one refname component from the front of refname.  Return
  * the length of the component found, or -1 if the component is not
@@ -2326,17 +2331,24 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
 
if (check_refname_format(r-name + 5, 0))
return;
 
-   lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
-   if (lock) {
-   unlink_or_warn(git_path(%s, r-name));
-   unlock_ref(lock);
-   try_remove_empty_parents(r-name);
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_delete(transaction, r-name, r-sha1,
+  REF_ISPRUNING, 1) ||
+   ref_transaction_commit(transaction, NULL, err)) {
+   ref_transaction_rollback(transaction);
+   warning(prune_ref: %s, err.buf);
+   strbuf_release(err);
+   return;
}
+   ref_transaction_free(transaction);
+   try_remove_empty_parents(r-name);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3512,9 +3524,10 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
struct ref_update *update = updates[i];
 
if (update-lock) {
-   delnames[delnum++] = update-lock-ref_name;
ret |= delete_ref_loose(update-lock, update-type,
err);
+   if (!(update-flags  REF_ISPRUNING))
+   delnames[delnum++] = update-lock-ref_name;
}
}
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only

[Ronnie Sahlberg]

Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.

At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/refs.c b/refs.c
index 69ad51d..f458ff9 100644
--- a/refs.c
+++ b/refs.c
@@ -33,6 +33,10 @@ static inline int bad_ref_char(int ch)
  *  pruned.
  */
 #define REF_ISPRUNING  0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ *  need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
 
 /*
  * Try to read one refname component from the front of refname.  Return
@@ -3346,6 +3350,9 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
if (transaction-status != REF_TRANSACTION_OPEN)
die(BUG: update on transaction that is not open);
 
+   if (flags  REF_ISPACKONLY)
+   die(BUG: REF_ISPACKONLY can not be used with updates);
+
update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
@@ -3370,6 +3377,9 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
if (transaction-status != REF_TRANSACTION_OPEN)
die(BUG: create on transaction that is not open);
 
+   if (flags  REF_ISPACKONLY)
+   die(BUG: REF_ISPACKONLY can not be used with creates);
+
update = add_update(transaction, refname);
 
hashcpy(update-new_sha1, new_sha1);
@@ -3483,10 +3493,20 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
if (ret)
goto cleanup;
 
+   for (i = 0; i  n; i++) {
+   struct ref_update *update = updates[i];
+
+   if (update-flags  REF_ISPACKONLY)
+   delnames[delnum++] = update-refname;
+   }
+
/* Acquire all locks while verifying old values */
for (i = 0; i  n; i++) {
struct ref_update *update = updates[i];
 
+   if (update-flags  REF_ISPACKONLY)
+   continue;
+
update-lock = lock_ref_sha1_basic(update-refname,
   (update-have_old ?
update-old_sha1 :
@@ -3524,6 +3544,9 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
for (i = 0; i  n; i++) {
struct ref_update *update = updates[i];
 
+   if (update-flags  REF_ISPACKONLY)
+   continue;
+
if (update-lock) {
ret |= delete_ref_loose(update-lock, update-type,
err);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 40/44] refs.c: call lock_ref_sha1_basic directly from commit

[Ronnie Sahlberg]

Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 53fdbd1..69ad51d 100644
--- a/refs.c
+++ b/refs.c
@@ -3487,12 +3487,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
for (i = 0; i  n; i++) {
struct ref_update *update = updates[i];
 
-   update-lock = lock_any_ref_for_update(update-refname,
-  (update-have_old ?
-   update-old_sha1 :
-   NULL),
-  update-flags,
-  update-type);
+   update-lock = lock_ref_sha1_basic(update-refname,
+  (update-have_old ?
+   update-old_sha1 :
+   NULL),
+  update-flags,
+  update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 37/44] refs.c: pass NULL as *flags to read_ref_full

[Ronnie Sahlberg]

We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 1977640..8105ee6 100644
--- a/refs.c
+++ b/refs.c
@@ -2612,7 +2612,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
goto rollback;
}
 
-   if (!read_ref_full(newrefname, sha1, 1, flag) 
+   if (!read_ref_full(newrefname, sha1, 1, NULL) 
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s, 
newrefname))) {
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 42/44] refs.c: pass a skip list to name_conflict_fn

[Ronnie Sahlberg]

Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m

For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 43 +--
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index f458ff9..c940509 100644
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+   const char **skip;
+   int skipnum;
 };
 
 static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
 {
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+   int i;
+   for(i = 0; i  data-skipnum; i++) {
+   if (!strcmp(entry-name, data-skip[i])) {
+   return 0;
+   }
+   }
if (data-oldrefname  !strcmp(data-oldrefname, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, void 
*cb_data)
  * conflicting with the name of an existing reference in dir.  If
  * oldrefname is non-NULL, ignore potential conflicts with oldrefname
  * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
  */
 static int is_refname_available(const char *refname, const char *oldrefname,
-   struct ref_dir *dir)
+   struct ref_dir *dir,
+   const char **skip, int skipnum)
 {
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+   data.skip = skip;
+   data.skipnum = skipnum;
 
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2037,7 +2051,8 @@ int dwim_log(const char *str, int len, unsigned char 
*sha1, char **log)
 
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
-   int flags, int *type_p)
+   int flags, int *type_p,
+   const char **skip, int skipnum)
 {
char *ref_file;
const char *orig_refname = refname;
@@ -2084,7 +2099,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
 * name is a proper prefix of our refname.
 */
if (missing 
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache))) 
{
+!is_refname_available(refname, NULL,
+  get_packed_refs(ref_cache),
+  skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2142,7 +2159,7 @@ struct ref_lock *lock_any_ref_for_update(const char 
*refname,
 const unsigned char *old_sha1,
 int flags, int *type_p)
 {
-   return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+   return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
 }
 
 /*
@@ -2592,6 +2609,9 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
 
+   if (!strcmp(oldrefname, newrefname))
+   return 0;
+
if (log  S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
 
@@ -2602,10 +2622,12 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
 
-   if (!is_refname_available(newrefname, oldrefname, 
get_packed_refs(ref_cache)))
+   if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(ref_cache), NULL, 0))
return 1;
 
-   if (!is_refname_available(newrefname, oldrefname, 

[PATCH v8 38/44] refs.c: pack all refs before we start to rename a ref

[Ronnie Sahlberg]

This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c| 3 +++
 t/t3200-branch.sh | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 8105ee6..79d95db 100644
--- a/refs.c
+++ b/refs.c
@@ -2607,6 +2607,9 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
return error(unable to move logfile logs/%s to 
TMP_RENAMED_LOG: %s,
oldrefname, strerror(errno));
 
+   if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+   return error(unable to pack refs);
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error(unable to delete old %s, oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master 
test_must_fail git branch -m master2 master3 
git symbolic-ref refs/heads/master2 
-   test_path_is_file .git/refs/heads/master 
+   test_path_is_missing .git/refs/heads/master 
test_path_is_missing .git/refs/heads/master3
 '
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 35/44] refs.c: make delete_ref use a transaction

[Ronnie Sahlberg]

Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 31 ++-
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 88ab012..e7bd95d 100644
--- a/refs.c
+++ b/refs.c
@@ -2488,11 +2488,6 @@ static int repack_without_refs(const char **refnames, 
int n, struct strbuf *err)
return ret;
 }
 
-static int repack_without_ref(const char *refname)
-{
-   return repack_without_refs(refname, 1, NULL);
-}
-
 static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf 
*err)
 {
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
@@ -2515,24 +2510,18 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag, struct strbuf *err)
 
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
-   struct ref_lock *lock;
-   int ret = 0, flag = 0;
+   struct ref_transaction *transaction;
 
-   lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
-   if (!lock)
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_delete(transaction, refname, sha1, delopt,
+  sha1  !is_null_sha1(sha1)) ||
+   ref_transaction_commit(transaction, NULL, NULL)) {
+   ref_transaction_rollback(transaction);
return 1;
-   ret |= delete_ref_loose(lock, flag, NULL);
-
-   /* removing the loose one could have resurrected an earlier
-* packed one.  Also, if it was not loose we need to repack
-* without it.
-*/
-   ret |= repack_without_ref(lock-ref_name);
-
-   unlink_or_warn(git_path(logs/%s, lock-ref_name));
-   clear_loose_ref_cache(ref_cache);
-   unlock_ref(lock);
-   return ret;
+   }
+   ref_transaction_free(transaction);
+   return 0;
 }
 
 /*
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 36/44] refs.c: pass the ref log message to _create/delete/update instead of _commit

[Ronnie Sahlberg]

Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 branch.c   |  6 +++---
 builtin/commit.c   |  4 ++--
 builtin/fetch.c| 10 --
 builtin/receive-pack.c |  4 ++--
 builtin/replace.c  |  4 ++--
 builtin/tag.c  |  4 ++--
 builtin/update-ref.c   | 13 +++--
 fast-import.c  |  8 
 refs.c | 34 +-
 refs.h |  8 
 sequencer.c|  4 ++--
 walker.c   |  6 +++---
 12 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/branch.c b/branch.c
index 8e58908..74d55e7 100644
--- a/branch.c
+++ b/branch.c
@@ -300,9 +300,9 @@ void create_branch(const char *head,
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, ref.buf, sha1,
-  null_sha1, 0, !forcing) ||
-   ref_transaction_commit(transaction, msg, err))
+   ref_transaction_update(transaction, ref.buf, sha1,
+  null_sha1, 0, !forcing, msg) ||
+   ref_transaction_commit(transaction, err))
die_errno(_(%s: failed to write ref: %s),
  ref.buf, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/commit.c b/builtin/commit.c
index 16fadbb..799a59f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
   current_head ?
   current_head-object.sha1 : NULL,
-  0, !!current_head) ||
-   ref_transaction_commit(transaction, sb.buf, err)) {
+  0, !!current_head, sb.buf) ||
+   ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5dbd0f0..3a849b0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
 {
+   char msg[1024];
+   char *rla = getenv(GIT_REFLOG_ACTION);
+
if (dry_run)
return 0;
+   if (!rla)
+   rla = default_rla.buf;
+   snprintf(msg, sizeof(msg), %s: %s, rla, action);
 
if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
-  ref-old_sha1, 0, check_old))
+  ref-old_sha1, 0, check_old, msg))
return STORE_REF_ERROR_OTHER;
 
return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
}
}
}
-   if (ref_transaction_commit(transaction, fetch_ref transaction, NULL))
+   if (ref_transaction_commit(transaction, NULL))
rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d580176..991c659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
return shallow error;
 
if (ref_transaction_update(transaction, namespaced_name,
-  new_sha1, old_sha1, 0, 1))
+  new_sha1, old_sha1, 0, 1, push))
return failed to update;
return NULL; /* good */
}
@@ -823,7 +823,7 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
-   if (ref_transaction_commit(transaction, push, err))
+   if (ref_transaction_commit(transaction, err))
error(%s, err.buf);
ref_transaction_free(transaction);
strbuf_release(err);
diff --git a/builtin/replace.c b/builtin/replace.c
index 91354a7..0c46b3a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
-  0, !is_null_sha1(prev)) ||
-   

[PATCH v8 14/44] replace.c: use the ref transaction functions for updates

[Ronnie Sahlberg]

Update replace.c to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/replace.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..ee34d5d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
unsigned char prev[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
 
if (snprintf(ref, sizeof(ref),
 refs/replace/%s,
@@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0, NULL);
-   if (!lock)
-   die(%s: cannot lock the ref, ref);
-   if (write_ref_sha1(lock, repl, NULL)  0)
-   die(%s: cannot update the ref, ref);
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref, repl, prev,
+  0, !is_null_sha1(prev)) ||
+   ref_transaction_commit(transaction, NULL, err))
+   die(%s: failed to replace ref: %s, ref, err.buf);
 
return 0;
 }
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 33/44] refs.c: remove lock_ref_sha1

[Ronnie Sahlberg]

lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial refs/
part of the ref path name, the initial refs/ that this caller had already
stripped off before calling lock_ref_sha1.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 7aa5512..f1ef940 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,15 +2126,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
return NULL;
 }
 
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char 
*old_sha1)
-{
-   char refpath[PATH_MAX];
-   if (check_refname_format(refname, 0))
-   return NULL;
-   strcpy(refpath, mkpath(refs/%s, refname));
-   return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
 struct ref_lock *lock_any_ref_for_update(const char *refname,
 const unsigned char *old_sha1,
 int flags, int *type_p)
@@ -2335,8 +2326,12 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-   struct ref_lock *lock = lock_ref_sha1(r-name + 5, r-sha1);
+   struct ref_lock *lock;
+
+   if (check_refname_format(r-name + 5, 0))
+   return;
 
+   lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path(%s, r-name));
unlock_ref(lock);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 05/44] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

[Ronnie Sahlberg]

Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index c2b720e..072e3e9 100644
--- a/refs.c
+++ b/refs.c
@@ -3408,6 +3408,7 @@ static int ref_update_compare(const void *r1, const void 
*r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+   struct strbuf *err,
enum action_on_err onerr)
 {
int i;
@@ -3415,6 +3416,9 @@ static int ref_update_reject_duplicates(struct ref_update 
**updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+   if (err)
+   strbuf_addf(err, str, updates[i]-refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3445,7 +3449,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
-   ret = ref_update_reject_duplicates(updates, n, onerr);
+   ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 09/44] refs.c: remove the onerr argument to ref_transaction_commit

[Ronnie Sahlberg]

Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  3 +--
 refs.c   | 22 +++---
 refs.h   |  3 +--
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   if (ref_transaction_commit(transaction, msg, err,
-  UPDATE_REFS_QUIET_ON_ERR))
+   if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
return 0;
}
diff --git a/refs.c b/refs.c
index 18dff1b..bc21060 100644
--- a/refs.c
+++ b/refs.c
@@ -3416,8 +3416,7 @@ static int ref_update_compare(const void *r1, const void 
*r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
-   struct strbuf *err,
-   enum action_on_err onerr)
+   struct strbuf *err)
 {
int i;
for (i = 1; i  n; i++)
@@ -3427,22 +3426,13 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]-refname);
 
-   switch (onerr) {
-   case UPDATE_REFS_MSG_ON_ERR:
-   error(str, updates[i]-refname); break;
-   case UPDATE_REFS_DIE_ON_ERR:
-   die(str, updates[i]-refname); break;
-   case UPDATE_REFS_QUIET_ON_ERR:
-   break;
-   }
return 1;
}
return 0;
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, struct strbuf *err,
-  enum action_on_err onerr)
+  const char *msg, struct strbuf *err)
 {
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3457,7 +3447,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
-   ret = ref_update_reject_duplicates(updates, n, err, onerr);
+   ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
 
@@ -3469,7 +3459,8 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   (update-have_old ?
update-old_sha1 : NULL),
   update-flags,
-  update-type, onerr);
+  update-type,
+  UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
@@ -3487,7 +3478,8 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
ret = update_ref_write(msg,
   update-refname,
   update-new_sha1,
-  update-lock, err, onerr);
+  update-lock, err,
+  UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 25ae110..555ee59 100644
--- a/refs.h
+++ b/refs.h
@@ -278,8 +278,7 @@ void ref_transaction_delete(struct ref_transaction 
*transaction,
  * the transaction failed. The string does not end in newline.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, struct strbuf *err,
-  enum action_on_err onerr);
+  const char *msg, struct strbuf *err);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
2.0.0.rc3.477.g0f8edf7

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

[PATCH v8 15/44] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/commit.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d28505a..592f019 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1581,11 +1581,12 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
-   struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
 
if (argc == 2  !strcmp(argv[1], -h))
usage_with_options(builtin_commit_usage, 
builtin_commit_options);
@@ -1707,16 +1708,6 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
 
-   ref_lock = lock_any_ref_for_update(HEAD,
-  !current_head
-  ? NULL
-  : current_head-object.sha1,
-  0, NULL);
-   if (!ref_lock) {
-   rollback_index_files();
-   die(_(cannot lock HEAD ref));
-   }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
@@ -1725,9 +1716,15 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
 
-   if (write_ref_sha1(ref_lock, sha1, sb.buf)  0) {
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, HEAD, sha1,
+  current_head ?
+  current_head-object.sha1 : NULL,
+  0, !!current_head) ||
+   ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
-   die(_(cannot update HEAD ref));
+   die(%s, err.buf);
}
 
unlink(git_path(CHERRY_PICK_HEAD));
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 18/44] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.

If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.

This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existsing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 branch.c | 30 --
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..2aa5c82 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
   int force, int reflog, int clobber_head,
   int quiet, enum branch_track track)
 {
-   struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_(Not a valid branch point: '%s'.), start_name);
hashcpy(sha1, commit-object.sha1);
 
-   if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
-   if (!lock)
-   die_errno(_(Failed to lock ref for update));
-   }
-
-   if (reflog)
-   log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, branch: Reset to %s,
 start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, branch: Created from %s,
 start_name);
 
+   if (reflog)
+   log_all_ref_updates = 1;
+
+   if (!dont_change_ref) {
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
+
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref.buf, sha1,
+  null_sha1, 0, !forcing) ||
+   ref_transaction_commit(transaction, msg, err))
+   die_errno(_(%s: failed to write ref: %s),
+ ref.buf, err.buf);
+   }
+
if (real_ref  track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
-   if (!dont_change_ref)
-   if (write_ref_sha1(lock, sha1, msg)  0)
-   die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
 }
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

[Ronnie Sahlberg]

Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 31 ++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 600f9b3..f11f832 100644
--- a/refs.c
+++ b/refs.c
@@ -3308,6 +3308,12 @@ struct ref_update {
const char refname[FLEX_ARRAY];
 };
 
+enum ref_transaction_status {
+   REF_TRANSACTION_OPEN   = 0,
+   REF_TRANSACTION_CLOSED = 1,
+   REF_TRANSACTION_ERROR  = 2,
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -3317,6 +3323,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+   enum ref_transaction_status status;
 };
 
 struct ref_transaction *ref_transaction_begin(void)
@@ -3340,6 +3347,11 @@ void ref_transaction_free(struct ref_transaction 
*transaction)
 
 void ref_transaction_rollback(struct ref_transaction *transaction)
 {
+   if (!transaction)
+   return;
+
+   transaction-status = REF_TRANSACTION_ERROR;
+
ref_transaction_free(transaction);
 }
 
@@ -3366,6 +3378,9 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
if (have_old  !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
 
+   if (transaction-status != REF_TRANSACTION_OPEN)
+   die(BUG: update on transaction that is not open);
+
update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
@@ -3385,6 +3400,9 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
if (!new_sha1 || is_null_sha1(new_sha1))
die(BUG: create ref with null new_sha1);
 
+   if (transaction-status != REF_TRANSACTION_OPEN)
+   die(BUG: create on transaction that is not open);
+
update = add_update(transaction, refname);
 
hashcpy(update-new_sha1, new_sha1);
@@ -3404,6 +3422,9 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
if (have_old  !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
 
+   if (transaction-status != REF_TRANSACTION_OPEN)
+   die(BUG: delete on transaction that is not open);
+
update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
@@ -3474,8 +3495,13 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
 
-   if (!n)
+   if (transaction-status != REF_TRANSACTION_OPEN)
+   die(BUG: commit on transaction that is not open);
+
+   if (!n) {
+   transaction-status = REF_TRANSACTION_CLOSED;
return 0;
+   }
 
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3538,6 +3564,9 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
clear_loose_ref_cache(ref_cache);
 
 cleanup:
+   transaction-status = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i  n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 13/44] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

Change tag.c to use ref transactions for all ref updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/tag.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..bf2a5c3 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
-   struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+   struct ref_transaction *transaction;
+   struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
-   if (!lock)
-   die(_(%s: cannot lock the ref), ref.buf);
-   if (write_ref_sha1(lock, object, NULL)  0)
-   die(_(%s: cannot update the ref), ref.buf);
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref.buf, object, prev,
+  0, !is_null_sha1(prev)) ||
+   ref_transaction_commit(transaction, NULL, err))
+   die(_(%s: cannot update the ref: %s), ref.buf, err.buf);
if (force  !is_null_sha1(prev)  hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag, 
find_unique_abbrev(prev, DEFAULT_ABBREV));
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 23/44] fetch.c: change s_update_ref to use a ref transaction

[Ronnie Sahlberg]

Change s_update_ref to use a ref transaction for the ref update.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/fetch.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..b41d7b7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
 {
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
-   static struct ref_lock *lock;
+   struct ref_transaction *transaction;
 
if (dry_run)
return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
snprintf(msg, sizeof(msg), %s: %s, rla, action);
 
errno = 0;
-   lock = lock_any_ref_for_update(ref-name,
-  check_old ? ref-old_sha1 : NULL,
-  0, NULL);
-   if (!lock)
-   return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
-   if (write_ref_sha1(lock, ref-new_sha1, msg)  0)
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref-name, ref-new_sha1,
+  ref-old_sha1, 0, check_old) ||
+   ref_transaction_commit(transaction, msg, NULL)) {
+   ref_transaction_rollback(transaction);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
+   }
+   ref_transaction_free(transaction);
return 0;
 }
 
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 10/44] refs.c: change ref_transaction_update() to do error checking and return status

[Ronnie Sahlberg]

Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future.

Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c | 10 ++
 refs.c   |  9 +++--
 refs.h   | 13 -
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..59c4d6b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
 
-   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old))
+   die(update %s: failed, refname);
 
update_flags = 0;
free(refname);
@@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
 
-   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old))
+   die(failed transaction update for %s, refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index bc21060..e8c2345 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,19 +3353,24 @@ static struct ref_update *add_update(struct 
ref_transaction *transaction,
return update;
 }
 
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
int flags, int have_old)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
+
+   if (have_old  !old_sha1)
+   die(BUG: have_old is true but old_sha1 is NULL);
 
+   update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+   return 0;
 }
 
 void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 555ee59..fd8d731 100644
--- a/refs.h
+++ b/refs.h
@@ -242,12 +242,15 @@ void ref_transaction_rollback(struct ref_transaction 
*transaction);
  * be deleted.  If have_old is true, then old_sha1 holds the value
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
  */
-void ref_transaction_update(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   const unsigned char *old_sha1,
-   int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  const unsigned char *old_sha1,
+  int flags, int have_old);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 12/44] refs.c: ref_transaction_delete to check for error and return status

[Ronnie Sahlberg]

Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  5 +++--
 refs.c   | 15 ++-
 refs.h   | 11 +++
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0924502..2a51df1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,8 +257,9 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(delete %s: extra input: %s, refname, next);
 
-   ref_transaction_delete(transaction, refname, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_delete(transaction, refname, old_sha1,
+  update_flags, have_old))
+   die(failed transaction delete for %s, refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index f38d3d6..6e5e940 100644
--- a/refs.c
+++ b/refs.c
@@ -3392,19 +3392,24 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
return 0;
 }
 
-void ref_transaction_delete(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *old_sha1,
+  int flags, int have_old)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
 
+   if (have_old  !old_sha1)
+   die(BUG: have_old is true but old_sha1 is NULL);
+
+   update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update-old_sha1, old_sha1);
}
+   return 0;
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index e3bdd57..cba2f3d 100644
--- a/refs.h
+++ b/refs.h
@@ -270,11 +270,14 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
  * Add a reference deletion to transaction.  If have_old is true, then
  * old_sha1 holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
  */
-void ref_transaction_delete(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *old_sha1,
+  int flags, int have_old);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 07/44] refs.c: make update_ref_write update a strbuf on failure

[Ronnie Sahlberg]

Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.

Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index a150520..18dff1b 100644
--- a/refs.c
+++ b/refs.c
@@ -3273,10 +3273,13 @@ static struct ref_lock *update_ref_lock(const char 
*refname,
 
 static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
-   enum action_on_err onerr)
+   struct strbuf *err, enum action_on_err onerr)
 {
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
+   if (err)
+   strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3402,7 +3405,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
-   return update_ref_write(action, refname, sha1, lock, onerr);
+   return update_ref_write(action, refname, sha1, lock, NULL, onerr);
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
@@ -3484,7 +3487,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
ret = update_ref_write(msg,
   update-refname,
   update-new_sha1,
-  update-lock, onerr);
+  update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
-- 
2.0.0.rc3.477.g0f8edf7

--
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 v8 00/44] Use ref transactions for all ref updates

[Ronnie Sahlberg]

This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions


This patch series is based on next and expands on the transaction API. It
converts all ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most of the ref updates to become
atomic when there are failures locking or writing to a ref.

This version completes the work to convert all ref updates to use transactions.
Now that all updates are through transactions I will start working on
cleaning up the reading of refs and to create an api for managing reflogs but
all that will go in a different patch series.

Version 8:
 - Updates after review by JN
 - Improve commit messages
 - Add a patch that adds an err argument to repack_without_refs
 - Add a patch that adds an err argument to delete_loose_ref
 - Better document that a _update/_delete/_create failure means the whole
   transaction has failed and needs rollback.

Version 7:
 - Updated commit messages per JNs review comments.
 - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
   exposed through refs.h

Version 6:
 - Convert all updates in refs.c to use transactions too.

Version 5:
 - Reword commit messages for having _create/_delete/_update returning
   success/failure. There are no conditions yet that return an error from
   these failures but there will be in the future. So we still check the
   return from these functions in the callers in preparation for this.
 - Don't leak memory by just passing a strbuf_detach() pointer to functions.
   Use obj.buf and explicitely strbuf_release the data afterwards.
 - Remove the function update_ref_lock.
 - Remove the function update_ref_write.
 - Track transaction status and die(BUG:) if we call _create/_delete/_update/
   _commit for a transaction that is not OPEN.

Version 4:
 - Rename patch series from Use ref transactions from most callers to
   Use ref transactions for all ref updates.
 - Convert all external ref writes to use transactions and make write_ref_sha1
   and lock_ref_sha1 static functions.
 - Change the ref commit and free handling so we no longer pass pointer to
   pointer to _commit. _commit no longer frees the transaction. The caller
   MUST call _free itself.
 - Change _commit to take a strbuf pointer instead of a char* for error
   reporting back to the caller.
 - Re-add the walker patch after fixing it.

Version 3:
 - Remove the walker patch for now. Walker needs more complex solution
   so defer it until the basics are done.
 - Remove the onerr argument to ref_transaction_commit(). All callers
   that need to die() on error now have to do this explicitely.
 - Pass an error string from ref_transaction_commit() back to the callers
   so that they can craft a nice error message upon failures.
 - Make ref_transaction_rollback() accept NULL as argument.
 - Change ref_transaction_commit() to take a pointer to pointer argument for
   the transaction and have it clear the callers pointer to NULL when
   invoked. This allows for much nicer handling of transaction rollback on
   failure.
Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head-object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.



Ronnie Sahlberg (44):
  refs.c: constify the sha arguments for
ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: add a strbuf argument to ref_transaction_commit for error
logging
  refs.c: add an err argument to repack_without_refs
  refs.c: make ref_update_reject_duplicates take a strbuf argument for
errors
  refs.c: add an err argument ro delete_loose_ref
  refs.c: make update_ref_write 

[PATCH v8 27/44] walker.c: use ref transaction for ref updates

[Ronnie Sahlberg]

Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.

This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.

Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 walker.c | 51 ++-
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/walker.c b/walker.c
index 1dd86b8..6044ccf 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target, 
const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
 const char **write_ref, const char *write_ref_log_details)
 {
-   struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+   char ref_name[PATH_MAX];
+   struct strbuf err = STRBUF_INIT;
+   struct ref_transaction *transaction;
unsigned char *sha1 = xmalloc(targets * 20);
-   char *msg;
-   int ret;
+   char *msg = NULL;
int i;
 
save_commit_buffer = 0;
 
-   for (i = 0; i  targets; i++) {
-   if (!write_ref || !write_ref[i])
-   continue;
-
-   lock[i] = lock_ref_sha1(write_ref[i], NULL);
-   if (!lock[i]) {
-   error(Can't lock ref %s, write_ref[i]);
-   goto unlock_and_fail;
-   }
-   }
+   transaction = ref_transaction_begin();
+   if (!transaction)
+   return -1;
 
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char 
**target,
for (i = 0; i  targets; i++) {
if (interpret_target(walker, target[i], sha1[20 * i])) {
error(Could not interpret response from server '%s' as 
something to pull, target[i]);
-   goto unlock_and_fail;
+   goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
-   goto unlock_and_fail;
+   goto rollback_and_fail;
}
 
if (loop(walker))
-   goto unlock_and_fail;
+   goto rollback_and_fail;
 
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char 
**target,
for (i = 0; i  targets; i++) {
if (!write_ref || !write_ref[i])
continue;
-   ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch 
(unknown));
-   lock[i] = NULL;
-   if (ret)
-   goto unlock_and_fail;
+   sprintf(ref_name, refs/%s, write_ref[i]);
+   if (ref_transaction_update(transaction, ref_name,
+  sha1[20 * i], NULL,
+  0, 0))
+   goto rollback_and_fail;
+   }
+
+   if (ref_transaction_commit(transaction, msg ? msg : fetch (unknown),
+  err)) {
+   error(%s, err.buf);
+   goto rollback_and_fail;
}
-   free(msg);
 
+   free(msg);
return 0;
 
-unlock_and_fail:
-   for (i = 0; i  targets; i++)
-   if (lock[i])
-   unlock_ref(lock[i]);
+rollback_and_fail:
+   free(msg);
+   strbuf_release(err);
+   ref_transaction_free(transaction);
 
return -1;
 }
-- 
2.0.0.rc3.477.g0f8edf7

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