Query: Failed to suppress CC while sending patchset

[Pramod Gurav]

Hi,

I was trying to send a patchset to self just to test by suppressing
everyone in CC list of patch header. But it failed and the patch was
send to people in CC.

Is there anything wrong that I have done? My git verions is:

gpramod:git$ git --version
git version 1.7.9.5

Below is the command I used to send the patch and it follows the
messages until I pressed a Ctrl+c.

gpramod:linux-next$ git send-email --to=pramod.gurav@gmail.com
--suppress-cc=cc --suppress-cc=self --suppress-cc=author 000*
-mmc-msm_sdcc-Cleanups.patch
0001-mmc-msm_sdcc-Switch-to-using-managed-resources.patch
0002-mmc-msm_sdcc-Add-support-for-platform_driver-remove-.patch
0003-mmc-msm_sdcc-Replace-pr_-with-dev_.patch
0004-mmc-msm_sdcc-Remove-duplicate-check-around-dmares.patch
0005-mmc-msm_sdcc-Remove-unwanted-initializations-in-prob.patch
OK. Log says:
Server: secure.emailsrvr.com
MAIL FROM:
RCPT TO:
From: Pramod Gurav 
To: pramod.gurav@gmail.com
Subject: [PATCH 0/5] mmc: msm_sdcc: Cleanups
Date: Tue, 16 Sep 2014 16:45:41 +0530
Message-Id: <1410866146-15144-1-git-send-email-pramod.gu...@smartplayin.com>
X-Mailer: git-send-email 1.7.9.5

Result: 250 2.0.0 Ok: queued as 1CD0F1802A2

In git 1.7.0, the default has changed to --no-chain-reply-to
Set sendemail.chainreplyto configuration variable to true if
you want to keep --chain-reply-to as your default.
(body) Adding cc: David Brown  from line 'Cc:
David Brown '
(body) Adding cc: Daniel Walker  from line 'Cc:
Daniel Walker '
(body) Adding cc: Bryan Huntsman  from line
'Cc: Bryan Huntsman '
(body) Adding cc: Ulf Hansson  from line 'CC:
Ulf Hansson '
(body) Adding cc: linux-...@vger.kernel.org from line 'CC:
linux-...@vger.kernel.org'
(body) Adding cc: linux-arm-...@vger.kernel.org from line 'CC:
linux-arm-...@vger.kernel.org'
OK. Log says:
Server: secure.emailsrvr.com
MAIL FROM:
RCPT TO:
RCPT TO:
RCPT TO:
RCPT TO:
RCPT TO:
RCPT TO:
RCPT TO:
From: Pramod Gurav 
To: pramod.gurav@gmail.com
Cc: David Brown ,
Daniel Walker ,
Bryan Huntsman ,
Ulf Hansson ,
linux-...@vger.kernel.org,
linux-arm-...@vger.kernel.org
Subject: [PATCH 1/5] mmc: msm_sdcc: Switch to using managed resources
Date: Tue, 16 Sep 2014 16:45:42 +0530
Message-Id: <1410866146-15144-2-git-send-email-pramod.gu...@smartplayin.com>
X-Mailer: git-send-email 1.7.9.5
In-Reply-To: <1410866146-15144-1-git-send-email-pramod.gu...@smartplayin.com>
References: <1410866146-15144-1-git-send-email-pramod.gu...@smartplayin.com>

Result: 250 2.0.0 Ok: queued as B9A481803A0

(body) Adding cc: David Brown  from line 'Cc:
David Brown '
(body) Adding cc: Daniel Walker  from line 'Cc:
Daniel Walker '
(body) Adding cc: Bryan Huntsman  from line
'Cc: Bryan Huntsman '
(body) Adding cc: Ulf Hansson  from line 'CC:
Ulf Hansson '
(body) Adding cc: linux-...@vger.kernel.org from line 'CC:
linux-...@vger.kernel.org'
(body) Adding cc: linux-arm-...@vger.kernel.org from line 'CC:
linux-arm-...@vger.kernel.org'
^C

gpramod:linux-next$ stty: standard input: Input/output error

This is the header of my first patch:

>From f7ced72a85866e550ea2d76d8d7f9d1df88f5994 Mon Sep 17 00:00:00 2001
From: Pramod Gurav 
Date: Tue, 16 Sep 2014 15:41:37 +0530
Subject: [PATCH 1/5] mmc: msm_sdcc: Switch to using managed resources

This change makes changes to use managed version of ioremap, clk_get,
request_irq etc for clean unloading of modules. This does away with
lables to release these resources.

Cc: David Brown 
Cc: Daniel Walker 
Cc: Bryan Huntsman 
CC: Ulf Hansson 
CC: linux-...@vger.kernel.org
CC: linux-arm-...@vger.kernel.org
Signed-off-by: Pramod Gurav 
---
 drivers/mmc/host/msm_sdcc.c |   43 +--

-- 
Thanks and Regards
Pramod
--
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/2] fixup! refs: make rev-parse --quiet actually quiet

[David Aguilar]

---
This is a fixup for da/rev-parse-verify-quiet in pu
We now exit(128) and handle the "Log for XXX only has DDD entries" case.

 refs.c  |  2 +-
 sha1_name.c |  3 +++
 t/t1503-rev-parse-verify.sh | 10 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 447e339..9e405f9 100644
--- a/refs.c
+++ b/refs.c
@@ -3128,7 +3128,7 @@ int read_ref_at(const char *refname, unsigned int flags, 
unsigned long at_time,
 
if (!cb.reccnt) {
if (flags & GET_SHA1_QUIETLY)
-   exit(1);
+   exit(128);
else
die("Log for %s is empty.", refname);
}
diff --git a/sha1_name.c b/sha1_name.c
index d292e31..be9a9de 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -563,6 +563,9 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
"back to %s.", len, str,
show_date(co_time, co_tz, 
DATE_RFC2822));
else {
+   if (flags & GET_SHA1_QUIETLY) {
+   exit(128);
+   }
die("Log for '%.*s' only has %d entries.",
len, str, co_cnt);
}
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 4fe9f0e..7c3d830 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -86,12 +86,20 @@ test_expect_success 'fails silently when using -q' '
 test_expect_success 'fails silently when using -q with deleted reflogs' '
ref=$(git rev-parse HEAD) &&
: >.git/logs/refs/test &&
-   git update-ref -m "reflog message for refs/test" refs/test "$ref" &&
+   git update-ref -m "message for refs/test" refs/test "$ref" &&
git reflog delete --updateref --rewrite refs/test@{0} &&
test_must_fail git rev-parse -q --verify refs/test@{0} >error 2>&1 &&
test_must_be_empty error
 '
 
+test_expect_success 'fails silently when using -q with not enough reflogs' '
+   ref=$(git rev-parse HEAD) &&
+   : >.git/logs/refs/test2 &&
+   git update-ref -m "message for refs/test2" refs/test2 "$ref" &&
+   test_must_fail git rev-parse -q --verify refs/test2@{999} >error 2>&1 &&
+   test_must_be_empty error
+'
+
 test_expect_success 'no stdout output on error' '
test -z "$(git rev-parse --verify)" &&
test -z "$(git rev-parse --verify foo)" &&
-- 
2.1.0.248.g6401287.dirty

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

[PATCH 2/2] rev-parse: honor --quiet when asking for reflog dates that do not exist

[David Aguilar]

Make rev-parse --verify --quiet ref@{1.year.ago} when the reflog
does not go back that far succeed silently with --quiet.

Signed-off-by: David Aguilar 
---
 sha1_name.c | 19 ---
 t/t1503-rev-parse-verify.sh | 10 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index be9a9de..5836f94 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -514,8 +514,11 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
 
if (warn_ambiguous_refs &&
(refs_found > 1 ||
-!get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
-   warning(warn_msg, len, str);
+!get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY))) {
+   if (!(flags & GET_SHA1_QUIETLY)) {
+   warning(warn_msg, len, str);
+   }
+   }
 
if (reflog_len) {
int nth, i;
@@ -558,11 +561,13 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
len = 4;
}
}
-   if (at_time)
-   warning("Log for '%.*s' only goes "
-   "back to %s.", len, str,
-   show_date(co_time, co_tz, 
DATE_RFC2822));
-   else {
+   if (at_time) {
+   if (!(flags & GET_SHA1_QUIETLY)) {
+   warning("Log for '%.*s' only goes "
+   "back to %s.", len, str,
+   show_date(co_time, co_tz, 
DATE_RFC2822));
+   }
+   } else {
if (flags & GET_SHA1_QUIETLY) {
exit(128);
}
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 7c3d830..e7dece0 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -100,6 +100,16 @@ test_expect_success 'fails silently when using -q with not 
enough reflogs' '
test_must_be_empty error
 '
 
+test_expect_success 'succeeds silently when using -q with invalid dates' '
+   ref=$(git rev-parse HEAD) &&
+   : >.git/logs/refs/test3 &&
+   git update-ref -m "message for refs/test3" refs/test3 "$ref" &&
+   git rev-parse -q --verify refs/test3@{1.year.ago} >actual 2>error &&
+   test_must_be_empty error &&
+   echo "$ref" >expect &&
+   test_cmp expect actual
+'
+
 test_expect_success 'no stdout output on error' '
test -z "$(git rev-parse --verify)" &&
test -z "$(git rev-parse --verify foo)" &&
-- 
2.1.0.248.g6401287.dirty

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

Re: [PATCH v3] pretty: add %D format specifier

[Junio C Hamano]

Junio C Hamano  writes:

>> +test_expect_success 'clean log decoration' '
>> +git log --no-walk --tags --pretty="%H %D" --decorate=full >actual &&
>> +cat > +$head1 tag: refs/tags/tag2
>> +$head2 tag: refs/tags/message-one
>> +$old_head1 tag: refs/tags/message-two
>> +EOF
> ...
> Hmph.  I actually think the part that prepares the history makes
> sure that the output order of the commits is predictable by using
> test_commit and test_tick.  I see existing tests at the end (which
> is a sign that they were added more recently than the rest of the
> test script, and can indicate a careless addition) already has
> "sort", but we shouldn't have to sort.

Actually --tags may feed the tips in unspecified order and --no-walk
ensures the commits will be shown in that unspecified order, so for
a test that runs "log --no-walk --tags", sorting is unavoidable.

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

Re: [PATCH] compat-util: add _DEFAULT_SOURCE define

[Sergey Senozhatsky]

Hello,

On (09/15/14 12:02), Junio C Hamano wrote:
> Date: Mon, 15 Sep 2014 12:02:33 -0700
> From: Junio C Hamano 
> To: Sergey Senozhatsky 
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] compat-util: add _DEFAULT_SOURCE define
> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)
> 
> Sergey Senozhatsky  writes:
> 
> > glibc has deprecated the use of _BSD_SOURCE define
> >
> >   warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
> >
> > To make it easier to maintain a cross platform source code, that
> > warning can be suppressed by _DEFAULT_SOURCE.
> >
> > Define both _BSD_SOURCE, _DEFAULT_SOURCE and cleanup the build.
> 
> I can see you defined DEFAULT_SOURCE, but where did you do "cleanup
> the build"?  Or did you mean "define both (in order) to clean up"?

yes. sorry, I meant "in order to clean up". otherwise, the build is
a bit noisy:

[..]
CC base85.o
CC bisect.o
* new link flags
* new prefix flags
CC branch.o
CC blob.o
In file included from /usr/include/unistd.h:25:0,
 from git-compat-util.h:98,
 from cache.h:4,
 from blob.c:1:
/usr/include/features.h:148:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE 
are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
 # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
   ^
In file included from /usr/include/unistd.h:25:0,
 from git-compat-util.h:98,
 from branch.c:1:
/usr/include/features.h:148:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE 
are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
 # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
   ^
In file included from /usr/include/unistd.h:25:0,
 from git-compat-util.h:98,
 from cache.h:4,
 from base85.c:1:
/usr/include/features.h:148:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE 
are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
 # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
   ^
In file included from /usr/include/unistd.h:25:0,
 from git-compat-util.h:98,
 from cache.h:4,
 from bisect.c:1:
/usr/include/features.h:148:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE 
are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
 # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
   ^

[..]

> Just making sure we are not only seeing a patch half eaten by mail
> transport somewhere.

sure.

-ss

> >
> > Signed-off-by: Sergey Senozhatsky 
> > ---
> >  git-compat-util.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 4e7e3f8..08a9ee2 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -82,6 +82,7 @@
> >  #define _ALL_SOURCE 1
> >  #define _GNU_SOURCE 1
> >  #define _BSD_SOURCE 1
> > +#define _DEFAULT_SOURCE 1
> >  #define _NETBSD_SOURCE 1
> >  #define _SGI_SOURCE 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 v5 23/35] lockfile: avoid transitory invalid states

[Jonathan Nieder]

Michael Haggerty wrote:

> We could probably continue to use the filename field to encode the
> state by being careful to write characters 1..N-1 of the filename
> first, and then overwrite the NUL at filename[0] with the first
> character of the filename, but that would be awkward and error-prone.
> 
> So, instead of using the filename field to determine whether the
> lock_file object is active, add a new field "lock_file::active" for
> this purpose.

Nice.

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned 
> int flags, const struct
>  
>  struct lock_file {
>   struct lock_file *next;
> + volatile sig_atomic_t active;
>   int fd;
>   pid_t owner;
>   char on_list;
[...]
> +++ b/lockfile.c
> @@ -27,16 +27,19 @@
[...]
> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char 
> *path, int flags)
>   atexit(remove_lock_file);
>   }
>  
> + if (lk->active)
> + die("BUG: lock_file(\"%s\") called with an active lock_file 
> object",
> + path);

The error message doesn't make it entirely obvious to me what went
wrong.

Maybe something like

die("BUG: cannot lock_file(\"%s\") on active struct lock_file",
path);

lock_file already assumed on_list was initialized to zero but it
wasn't particularly obvious since everything else is blindly
scribbled over.  Probably worth mentioning in the API docs that
the lock_file should be zeroed before calling hold_lock_file_...

[...]
> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
>   if (rename(lk->filename, result_file))
>   goto rollback;
>  
> + lk->active = 0;
>   lk->filename[0] = 0;

Is it useful to set filename[0] any more?

It seems potentially fragile to set both, since new code can appear
that only sets or checks one or the other.  Would it make sense to
rename the filename field to make sure no new callers relying on the
filename[0] convention sneak in (with the new convention being that
the filename doesn't get cleared, to avoid problems)?

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 v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

> After commit_lock_file() is called, then the lock_file object is
> necessarily either committed or rolled back.  So there is no need to
> call rollback_lock_file() again in either of these cases.
>
> Signed-off-by: Michael Haggerty 

This seems to involve an unadvertised behavior change: before
it wouldn't call git_config_clear() on failure, and after the
patch it would.  Intended?

I think it would be clearer with the goto for exception handling
maintained (and just a second 'lock = NULL' assignment).
--
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] allowing hooks to ignore input?

[Junio C Hamano]

Johannes Sixt  writes:

> I think this is a good move. Hooks are written by users, who sometimes
> are not clueful enough.

Thanks for a sanity check.  I do not think it is about cluefulness
in this particular case.  A rule that is not meaningfully enforced
by reliably failing offenders is a rule that is hard to follow if a
clueful user wanted to.

This round comes with a test, but depending on the size of your pipe
buffer and context switching race, an unpatched Git may pass it
(reducing the test_seq number down to say 199 would certainly make
it pass without the fix most of the time).

-- >8 --
Subject: [PATCH] receive-pack: allow hooks to ignore its standard input stream

The pre-receive and post-receive hooks were designed to be an
improvement over old style update and post-update hooks, which take
the update information on their command line and are limited by the
command line length limit.  The same information is fed from the
standard input to pre/post-receive hooks instead to lift this
limitation.  It has been mandatory for these new style hooks to
consume the update information fully from the standard input stream.
Otherwise, they would risk killing the receive-pack process via
SIGPIPE.

If a hook does not want to look at all the information, it is easy
to send its standard input to /dev/null (perhaps a niche use of hook
might need to know only the fact that a push was made, without
having to know what objects have been pushed to update which refs),
and this has already been done by existing hooks that are written
carefully.

However, because there is no good way to consistently fail hooks
that do not consume the input fully (a small push may result in a
short update record that may fit within the pipe buffer, to which
the receive-pack process may manage to write before the hook has a
chance to exit without reading anything, which will not result in a
death-by-SIGPIPE of receive-pack), it can lead to a hard to diagnose
"once in a blue moon" phantom failure.

Lift this "hooks must consume their input fully" mandate.  A mandate
that is not enforced strictly is not helping us to catch mistakes in
hooks.  If a hook has a good reason to decide the outcome of its
operation without reading the information we feed it, let it do so
as it pleases.

Signed-off-by: Junio C Hamano 
---
 builtin/receive-pack.c  |  6 ++
 t/t5401-update-hooks.sh | 13 +
 2 files changed, 19 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f93ac45..516386f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -15,6 +15,7 @@
 #include "connected.h"
 #include "argv-array.h"
 #include "version.h"
+#include "sigchain.h"
 
 static const char receive_pack_usage[] = "git receive-pack ";
 
@@ -288,6 +289,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn 
feed, void *feed_sta
return code;
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
+
while (1) {
const char *buf;
size_t n;
@@ -299,6 +302,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn 
feed, void *feed_sta
close(proc.in);
if (use_sideband)
finish_async(&muxer);
+
+   sigchain_pop(SIGPIPE);
+
return finish_command(&proc);
 }
 
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 17bcb0b..7f278d8 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -135,4 +135,17 @@ test_expect_success 'send-pack stderr contains hook 
messages' '
test_cmp expect actual
 '
 
+test_expect_success 'pre-receive hook that forgets to read its input' '
+   write_script victim.git/hooks/pre-receive <<-\EOF &&
+   exit 0
+   EOF
+   rm -f victim.git/hooks/update victim.git/hooks/post-update &&
+
+   for v in $(test_seq 100 999)
+   do
+   git branch branch_$v master || return
+   done &&
+   git push ./victim.git "+refs/heads/*:refs/heads/*"
+'
+
 test_done
-- 
2.1.0-403-g099cf47

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

Re: [PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty 
> ---
>  fast-import.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

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

Re: [PATCH v5 20/35] api-lockfile: document edge cases

[Jonathan Nieder]

Michael Haggerty wrote:

> * Document the behavior of commit_lock_file() when it fails, namely
>   that it rolls back the lock_file object and sets errno
>   appropriately.
>
> * Document the behavior of rollback_lock_file() when called for a
>   lock_file object that has already been committed or rolled back,
>   namely that it is a NOOP.

I think this would be easier to read in a separate error handling
section.  That way, when writing new code using the lockfile API,
I can quickly find what functions to use and quickly find out what
the error handling conventions are --- each use of the documentation
wouldn't interfere with the other.

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 v5 19/35] commit_lock_file(): rollback lock file on failure to rename

[Jonathan Nieder]

Michael Haggerty wrote:

> If rename() fails, call rollback_lock_file() to delete the lock file
> (in case it is still present) and reset the filename field to the
> empty string so that the lockfile object is left in a valid state.

Can you spell out more what the goal is?  Is the idea to keep the
.lock file for a shorter period of time, so other processes can lock
that file before the current process exits?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back

[Jonathan Nieder]

Michael Haggerty wrote:

> If closing an open lockfile fails, then we cannot be sure of the
> contents of the lockfile

Is that true?  It seems more like a bug in close_lock_file: if it
fails, perhaps it should either set lk->fd back to fd or unlink the
lockfile itself.

What do other callers do on close_lock_file failure?

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 v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -312,6 +312,9 @@ int commit_lock_file(struct lock_file *lk)
>  {
>   char result_file[PATH_MAX];
>  
> + if (!lk->filename[0])
> + die("BUG: attempt to commit unlocked object");

Sure, this is fine instead of an assert() too. :)

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 v5 16/35] commit_lock_file(): inline temporary variable

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -311,12 +311,14 @@ int reopen_lock_file(struct lock_file *lk)
>  int commit_lock_file(struct lock_file *lk)
>  {
>   char result_file[PATH_MAX];
> - size_t i;
> +
>   if (lk->fd >= 0 && close_lock_file(lk))
>   return -1;
> +
>   strcpy(result_file, lk->filename);
> - i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
> - result_file[i] = 0;
> + /* remove ".lock": */
> + result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
> +

Same comment as the delete_ref_loose patch: the reader can gain some
piece of mind from an assertion at the top that makes sure filename is
valid (which it always should be, according to the API):

assert(lk->filename[0]);

It would also be nice to use the same xmemdupz-based code as in the
delete_ref_loose case (ideally via a helper), to avoid having to work
with a PATH_MAX-sized buffer.

Otherwise looks good.

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 v5 15/35] remove_lock_file(): call rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

>  lockfile.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)

Nice.

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

Re: [PATCH v5 14/35] lock_file(): exit early if lockfile cannot be opened

[Jonathan Nieder]

Michael Haggerty wrote:

>  lockfile.c | 23 +++
>  1 file changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Jonathan Nieder 
--
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] unblock and unignore SIGPIPE

[Junio C Hamano]

Patrick Reynolds  writes:

> Blocked and ignored signals -- but not caught signals -- are inherited
> across exec.  Some callers with sloppy signal-handling behavior can call
> git with SIGPIPE blocked or ignored, even non-deterministically.  When
> SIGPIPE is blocked or ignored, several git commands can run indefinitely,
> ignoring EPIPE returns from write() calls, even when the process that
> called them has gone away.  Our specific case involved a pipe of git
> diff-tree output to a script that reads a limited amount of diff data.
>
> In an ideal world, git would never be called with SIGPIPE blocked or
> ignored.  But in the real world, several real potential callers, including
> Perl, Apache, and Unicorn, sometimes spawn subprocesses with SIGPIPE
> ignored.  It is easier and more productive to harden git against this
> mistake than to clean it up in every potential parent process.
>
> Signed-off-by: Patrick Reynolds 

I missed this one when it was posted.

I have a suspicion that $gmane/256544 (relevant parties Cc'ed) is
cured by this (even though the other topic came much later than this
change)?

> diff --git a/git.c b/git.c
> index 9c49519..d6b221b 100644
> --- a/git.c
> +++ b/git.c
> @@ -611,6 +611,11 @@ int main(int argc, char **av)
>*/
>   sanitize_stdfds();
>  
> + /*
> +  * Make sure we aren't ignoring or blocking SIGPIPE.
> +  */
> + sanitize_signals();
> +
>   git_setup_gettext();
>  
>   trace_command_performance(argv);
> diff --git a/setup.c b/setup.c
> index 0a22f8b..7aa4b01 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -865,3 +865,14 @@ int daemonize(void)
>   return 0;
>  #endif
>  }
> +
> +/* un-ignore and un-block SIGPIPE */
> +void sanitize_signals(void)
> +{
> + sigset_t unblock;
> +
> + sigemptyset(&unblock);
> + sigaddset(&unblock, SIGPIPE);
> + sigprocmask(SIG_UNBLOCK, &unblock, NULL);
> + signal(SIGPIPE, SIG_DFL);

With the only caller in git.c, there is not a good reason that we
would want to have this as a global in a different file (I think the
patch merely follows the pattern of sanitize-fds, but that one has
to be called from many places).

After making the function static to git.c, it would also be a good
idea to rename it to match the comment at the sole callsite,
e.g. "restore_sigpipe_to_default()" or something.  Then the comment
at the callsite can be removed.

Later somebody else may want to add other signals handled similarly
for whatever reason (I do not think of any signal or any good reason
at this moment).  But they will have to come up with much better
justification than "the function name says 'sanitize'" if we named
it to something specific to SIGPIPE.  And that good justification
will guide us what kind of signals need to be "sanitized" and find a
better verb than "sanitize", if it ever happens.

> diff --git a/t/t0012-sigpipe.sh b/t/t0012-sigpipe.sh
> new file mode 100755
> index 000..213cde3
> --- /dev/null
> +++ b/t/t0012-sigpipe.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh

Hmmm, do we really need to allocate a new test number just for these
two tests, instead of folding it into an existing one?

> +test_expect_success 'create blob' '
> + test-genrandom foo 16384 >file &&
> + git add file
> +'
> +
> +large_git () {
> + for i in $(test_seq 1 100); do
> + git diff --staged --binary || return $?

Write it more like this:

for i in $(...)
do
git diff --cached --binary || return
done

to (1) avoid unnecessary semicolon before "do", (2) prefer the true
option name over a synonym, and (3) omit unnecessary $? that is
given to "return".

Summing them up, something like this squashed in would give us a
good end result, perhaps?

---
 cache.h|  1 -
 git.c  | 25 +
 setup.c| 11 ---
 t/t-basic.sh   | 17 +
 t/t0012-sigpipe.sh | 27 ---
 5 files changed, 38 insertions(+), 43 deletions(-)
 delete mode 100755 t/t0012-sigpipe.sh

diff --git a/cache.h b/cache.h
index 0a89fc1..fcb511d 100644
--- a/cache.h
+++ b/cache.h
@@ -463,7 +463,6 @@ extern int set_git_dir_init(const char *git_dir, const char 
*real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
-extern void sanitize_signals(void);
 extern int daemonize(void);
 
 #define alloc_nr(x) (((x)+16)*3/2)
diff --git a/git.c b/git.c
index d6b221b..c87bacd 100644
--- a/git.c
+++ b/git.c
@@ -592,6 +592,26 @@ static int run_argv(int *argcp, const char ***argv)
return done_alias;
 }
 
+/*
+ * Many parts of Git have subprograms communicate via pipe, expect the
+ * upstream of the pipe to die with SIGPIPE and the downstream process
+ * even knows to check and handle EPIPE correctly.  Some third-party
+ * programs that ignore or block SIGPIPE for their own reason forget
+ * to restore SIGPIPE handling to the default before spawning Git 

Re: [PATCH v5 12/35] prepare_index(): declare return value to be (const char *)

[Jonathan Nieder]

Michael Haggerty wrote:

> Declare the return value to be const to make it clear that we aren't
> giving callers permission to write over the string that it points at.
> (The return value is the filename field of a struct lock_file, which
> can be used by a signal handler at any time and therefore shouldn't be
> tampered with.)
>
> Signed-off-by: Michael Haggerty 
> ---
>  builtin/commit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Jonathan Nieder 

I wonder if we should just bite the bullet and make lock_file an
opaque struct with accessors for int fd and const char *filename.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename

[Jonathan Nieder]

Michael Haggerty wrote:

> It's bad manners. Especially since there could be a signal during the
> call to unlink_or_warn(), in which case the signal handler will see
> the wrong filename and delete the reference file, leaving the lockfile
> behind.
>
> So make our own copy to work with.

Nice.  Could this be a helper, to help others avoid a PATH_MAX
sized buffer too?

[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, 
> struct strbuf *err)
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
[...]
> + /*
> +  * loose.  The loose file name is the same as the
> +  * lockfile name, minus ".lock":
> +  */
> + char *loose_filename = xmemdupz(
> + lock->lk->filename,
> + strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);

Ronnie mentioned that this would misbehave in the
!lock->lk->filename[0] case, which shouldn't come up because
lock_ref_sha1_basic dies on error but seems worth futureproofing
against.  Maybe something like

assert(lock->lk->filename[0]);

would be enough to make the assumption obvious for people reading.

Otherwise looks good.

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 v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Jonathan Nieder]

Michael Haggerty wrote:

> There are a few places that use these values, so define constants for
> them.

Seems like a symptom of the API leaving out a useful helper (e.g.,
something that strips off the lock suffix and returns a memdupz'd
filename).

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, 
> struct stat *st);
>  #define REFRESH_IN_PORCELAIN 0x0020  /* user friendly output, not "needs 
> update" */
>  extern int refresh_index(struct index_state *, unsigned int flags, const 
> struct pathspec *pathspec, char *seen, const char *header_msg);
>  
> +/* String appended to a filename to derive the lockfile name: */
> +#define LOCK_SUFFIX ".lock"
> +#define LOCK_SUFFIX_LEN 5

My suspicion is that error handling would be better if fewer callers
outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems
like a step in the wrong direction.

Adding constants in lockfile.c would make sense, though.

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 v5 09/35] lockfile.c: document the various states of lock_file objects

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -4,6 +4,63 @@
>  #include "cache.h"
>  #include "sigchain.h"
>
> +/*
> + * File write-locks as used by Git.
> + *
> + * When a file at $FILENAME needs to be written, it is done as
> + * follows:

This overlaps a lot with the API doc, which makes me worry a little
about it going out of date.  Would improving the API doc help, or if
aspects are especially relevant here, is there a way to summarize them
more quickly to avoid some of the redundancy?

[...]
> + * A lock_file object can be in several states:

Would it make sense for this information to go near the definition of
'struct lock_file'?  That's where I'd start if I were looking for
information on what states a lock_file can be in.

My two cents,
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 v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Jonathan Nieder]

Michael Haggerty wrote:

> The ambiguity didn't have any ill effects, because lock_file objects
> cannot be removed from the lock_file_list anyway.  But it is
> unnecessary to leave this behavior inconsistent.

Nit: commit messages usually use present tense for current behavior
(and imperative for the behavior after the patch).

More importantly, the above + the diffstat don't leave me very happy
about the change.

Can you spell out more about the intended effect?  E.g., is this about
making sure other code is appropriately exercised to tolerate the
signal handler even when there are a lot of errors (which should make
debugging easier) or something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, 
> const char *path, int flags)
>   if (errno != ENOENT) {
>   if (flags & LOCK_DIE_ON_ERROR)
>   die("cannot open '%s' for copying", path);
> - close(fd);
> + rollback_lock_file(lk);
>   return error("cannot open '%s' for copying", path);

Makes sense.

Now that I'm here, I wonder a little at the error convention.  If the
caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to
use unable_to_lock_message?  What errno would they pass in the err
parameter?  Would callers want handle failure to acquire a lock
differently from other errors (e.g., by sleeping and trying again),
and if not, what is the optionally-die behavior in hold_lock_file
about?

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

Re: [PATCH v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted

[Jonathan Nieder]

Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty 
> ---
>  lockfile.c | 1 +
>  1 file changed, 1 insertion(+)

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

Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1

[Jonathan Nieder]

Jonathan Nieder wrote:
> Michael Haggerty wrote:

>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)
>>  return;
>>  
>>  if (lk->fd >= 0)
>> -close(lk->fd);
>> +close_lock_file(lk);
>
> Doesn't need to be guarded by the 'if'.

Err, yes it does.

Why doesn't close_lock_file bail out early when fd < 0?

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

Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)
>   return;
>  
>   if (lk->fd >= 0)
> - close(lk->fd);
> + close_lock_file(lk);

Doesn't need to be guarded by the 'if'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 04/35] rollback_lock_file(): exit early if lock is not active

[Jonathan Nieder]

Michael Haggerty wrote:

> Eliminate a layer of nesting.

Oh --- guess I should have been more patient. :)

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

Re: [PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly

[Jonathan Nieder]

Michael Haggerty wrote:

> It is only necessary to clear the lock_file's filename field if it was
> not already clear.
[...]
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -276,6 +276,6 @@ void rollback_lock_file(struct lock_file *lk)
>   if (lk->fd >= 0)
>   close(lk->fd);
>   unlink_or_warn(lk->filename);
> + lk->filename[0] = 0;
>   }
> - lk->filename[0] = 0;

Now that it does nothing when lk->filename[0] == '\0', this could be
de-indented a little:

if (!lk->filename[0])
return;
if (lk->fd >= 0)
close(lk->fd);
unlink_or_warn(lk->filename);
lk->filename[0] = 0;

This could technically double-close if interrupted by a signal that
tries to remove the file again, right?  Should this use
close_lock_file which sets fd to -1 before closing?

if (!lk->filename[0])
return;
close_lock_file(lk);
unlink_or_warn(lk->filename);
lk->filename[0] = 0;

With or without such changes,
Reviewed-by: Jonathan Nieder 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 00/23] Signed push

[Eric Sunshine]

On Tuesday, September 16, 2014, Junio C Hamano  wrote:
>
> Junio C Hamano  writes:
>
> > A failing test has been added at the end for smart HTTP.  It appears
> > that somewhere in the callchain "--signed" is forgotten and the
> > sending end not to send the certificate for some reason.  If
> > somebody with a fresh set of eyes can look into it, that would be
> > very much appreciated, as I do not expect I would have sufficient
> > concentration to dig it quickly for several days at least.
>
> I lied.  This is to replace the patch at the end (23/23).
>
> -- >8 --
> From: Junio C Hamano 
> Date: Mon, 15 Sep 2014 14:59:00 -0700
> Subject: [PATCH] push: teach smart-HTTP to pass "git push --signed" around
>
> The "--signed" option received by "git push" is first passed to the
> transport layer, which the native transport directly uses to notice
> that a push certificate needs to be sent.  When the transport-helper
> is involved, however, the option needs to be told to the helper with
> set_helper_option(), and the helepr needs to take necessary action.

s/helepr/helper/

> For the smart-HTTP helper, the "necessary action" involves spawning
> the "git send-pack" subprocess with the "--signed" option.
>
> Once the above all gets wired in, the smart-HTTP transport now can
> use the push certificate mechanism to authenticate its pushes.
>
> Add a test that is modeled after tests for the native transport in
> t5534-push-signed.sh to t5541-http-push-smart.sh.  Update the test
> Apache configuration to pass GNUPGHOME environment variable through.
>
> Signed-off-by: Junio C Hamano 
--
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] mailinfo: do not treat ">From" lines as in-body headers

[Jeff King]

On Tue, Sep 16, 2014 at 11:41:08AM -0700, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> >> I think you forgot to "git add" mbox.h. That being said, if we did go
> >> this route, I do not see any reason to share the code at all. This can
> >> be purely a mailinfo.c thing.
> >
> > OK.  A reroll coming today when I find time.
> 
> -- >8 --
> From: Jeff King 
> Date: Sat, 13 Sep 2014 21:30:38 -0400
> Subject: [PATCH] mailinfo: make ">From" in-body header check more robust
>[...]

This looks good to me. Thanks.

-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 v5 02/35] api-lockfile: expand the documentation

[Jonathan Nieder]

Michael Haggerty wrote:

> Document a couple more functions and the flags argument as used by
> hold_lock_file_for_update() and hold_lock_file_for_append().

Thanks.

[...]
> --- a/Documentation/technical/api-lockfile.txt
> +++ b/Documentation/technical/api-lockfile.txt
> @@ -28,9 +28,39 @@ hold_lock_file_for_update::
>   the final destination (e.g. `$GIT_DIR/index`) and a flag
>   `die_on_error`.  Attempt to create a lockfile for the
>   destination and return the file descriptor for writing
> - to the file.  If `die_on_error` flag is true, it dies if
> - a lock is already taken for the file; otherwise it
> - returns a negative integer to the caller on failure.
> + to the file.  The flags parameter is a combination of
> ++
> +--

Context: this document has structure

lockfile API


Explanation of purpose (nice!).

The functions
-

Quick descriptions of each of the four functions
`hold_lock_file_for_update`, `commit_lock_file`,
`rollback_lock_file`, `close_lock_file`.

Reminder about lifetime of the lock_file structure.

Description of cleanup convention (thou shalt either
commit or roll back; if you forget to, the atexit
handler will roll back for you).

Long warning about the harder use cases.  The above
"thou shalt" was a lie --- you can also
close_lock_file if you know what you're doing
[jn: why is that function part of the public API?].

What's nice about the existing structure is that you can get
a sense of how to use the API at a glance.  Would there be a
way to add this extra information while preserving that property?

E.g.:

lockfile API


Nice brief explanation of purpose ("is this the API
I want to use?"), as before.

Calling sequence


The caller:

* Allocates a variable `struct lock_file lock` in the bss
section or heap.  Because the `lock_file` structure is used
in an `atexit(3)` handler, its storage has to stay
throughout the life of the program.  It cannot be an auto
variable allocated on the stack.

* Attempts to create a lockfile by passing that variable and
the filename of the final destination (e.g. `$GIT_DIR/index`)
to `hold_lock_file_for_update` or `hold_lock_file_for_append`.
+
If the `die_on_error` flag is true, git dies if a lock is
already taken for the file.

* Writes new content for the destination file by writing to
`lock->fd`.

When finished writing, the caller can:

* Close the file descriptor and rename the lockfile to
its final destination by calling `commit_lock_file`.

* Close the file descriptor and remove the lockfile by
calling `rollback_lock_file`.

* Close the file descriptor without removing or renaming
the lockfile by calling `close_lock_file`.

If you do not call one of `commit_lock_file`,
`rollback_lock_file`, and `close_lock_file` and instead
simply `exit(3)` from the program, an `atexit(3)` handler will
close and remove the lockfile.

You should never call `close(2)` on `lock->fd` yourself~
Otherwise the ...

Error handling
--

Functions return 0 on success, -1 on failure.  errno is?
isn't? meaningful on error.

... description of unable_to_lock_error and unable_to_lock_die
here ...

Flags
-

LOCK_NODEREF::

Usually symbolic links in the destination path are
resolved and the lockfile is created by adding ".lock"
to the resolved path.  If `LOCK_NODEREF` is set, then
the lockfile is created by adding ".lock" to the path
argument itself.

What is the user-visible effect of that flag?  When would I want
to pass that flag, and when wouldn't I?

LOCK_DIE_ON_ERROR::

If a lock is already taken for the file, `die()` with
an error message.  If this option is not specified,
trying to hold a lock file that is already taken will
return -1 to the caller.

Sensible?
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 v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()

[Jonathan Nieder]

Michael Haggerty wrote:

> This function is used for other things besides the index, so rename it
> accordingly.

Makes sense.

[...]
>  builtin/update-index.c | 2 +-
>  cache.h| 2 +-
>  lockfile.c | 6 +++---
>  refs.c | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Jonathan Nieder 
--
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 v5 32/35] Extract a function commit_lock_file_to()

[Michael Haggerty]

commit_locked_index(), when writing to an alternate index file,
duplicates (poorly) the code in commit_lock_file(). And anyway, it
shouldn't have to know so much about the internal workings of lockfile
objects. So extract a new function commit_lock_file_to() that does the
work common to the two functions, and call it from both
commit_lock_file() and commit_locked_index().

Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 14 
 cache.h  |  1 +
 lockfile.c   | 39 +---
 read-cache.c | 13 +++
 4 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index 2514559..3ee4299 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -73,6 +73,12 @@ commit_lock_file::
`commit_lock_file()` for a `lock_file` object that is not
currently locked.
 
+commit_lock_file_to::
+
+   Like `commit_lock_file()`, except that it takes an explicit
+   `path` argument to which the lockfile should be renamed. The
+   `path` must be on the same filesystem as the lock file.
+
 rollback_lock_file::
 
Take a pointer to the `struct lock_file` initialized
@@ -91,10 +97,10 @@ Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
 cannot be an auto variable allocated on the stack.
 
-Call `commit_lock_file()` or `rollback_lock_file()` when you are
-done writing to the file descriptor.  If you do not call either
-and simply `exit(3)` from the program, an `atexit(3)` handler
-will close and remove the lockfile.
+Call `commit_lock_file()`, `commit_lock_file_to()`, or
+`rollback_lock_file()` when you are done writing to the file
+descriptor. If you do not call either and simply `exit(3)` from the
+program, an `atexit(3)` handler will close and remove the lockfile.
 
 If you need to close the file descriptor you obtained from
 `hold_lock_file_for_update` function yourself, do so by calling
diff --git a/cache.h b/cache.h
index 307cc8e..45688d5 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int 
err,
 extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, 
int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, 
int);
+extern int commit_lock_file_to(struct lock_file *, const char *path);
 extern int commit_lock_file(struct lock_file *);
 extern int reopen_lock_file(struct lock_file *);
 extern void update_index_if_able(struct index_state *, struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 3d5ab4f..480c2ba 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -58,8 +58,9 @@
  * - Locked, lockfile closed (after close_lock_file()).  Same as the
  *   previous state, except that the lockfile is closed and fd is -1.
  *
- * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
- *   failed attempt to lock).  In this state:
+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),
+ *   rollback_lock_file(), or a failed attempt to lock).  In this
+ *   state:
  *   - active is unset
  *   - filename is empty (usually, though there are transitory
  * states in which this condition doesn't hold)
@@ -290,24 +291,16 @@ int reopen_lock_file(struct lock_file *lk)
return lk->fd;
 }
 
-int commit_lock_file(struct lock_file *lk)
+int commit_lock_file_to(struct lock_file *lk, const char *path)
 {
-   static struct strbuf result_file = STRBUF_INIT;
int save_errno;
-   int err;
 
if (!lk->active)
-   die("BUG: attempt to commit unlocked object");
+   die("BUG: attempt to commit unlocked object to \"%s\"", path);
 
if (lk->fd >= 0 && close_lock_file(lk))
goto rollback;
-
-   /* remove ".lock": */
-   strbuf_add(&result_file, lk->filename.buf,
-  lk->filename.len - LOCK_SUFFIX_LEN);
-   err = rename(lk->filename.buf, result_file.buf);
-   strbuf_reset(&result_file);
-   if (err)
+   if (rename(lk->filename.buf, path))
goto rollback;
lk->active = 0;
strbuf_reset(&lk->filename);
@@ -320,6 +313,26 @@ rollback:
return -1;
 }
 
+int commit_lock_file(struct lock_file *lk)
+{
+   static struct strbuf result_file = STRBUF_INIT;
+   int err;
+
+   if (!lk->active)
+   die("BUG: attempt to commit unlocked object");
+
+   if (lk->filename.len <= LOCK_SUFFIX_LEN ||
+   strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, 
LOCK_SUFFIX))
+   die("BUG: lockfile filename corrupt");
+
+   /* remove ".lock": */
+   strbuf_add(&result_file, lk->filename.buf,
+

[PATCH v5 24/35] struct lock_file: declare some fields volatile

[Michael Haggerty]

The function remove_lock_file_on_signal() is used as a signal handler.
It is not realistic to make the signal handler conform strictly to the
C standard, which is very restrictive about what a signal handler is
allowed to do.  But let's increase the likelihood that it will work:

The lock_file_list global variable and several fields from struct
lock_file are used by the signal handler.  Declare those values
"volatile" to (1) force the main process to write the values to RAM
promptly, and (2) prevent updates to these fields from being reordered
in a way that leaves an opportunity for a jump to the signal handler
while the object is in an inconsistent state.

We don't mark the filename field volatile because that would prevent
the use of strcpy(), and it is anyway unlikely that a compiler
re-orders a strcpy() call across other expressions.  So in practice it
should be possible to get away without "volatile" in the "filename"
case.

Suggested-by: Johannes Sixt 
Signed-off-by: Michael Haggerty 
---
 cache.h| 6 +++---
 lockfile.c | 2 +-
 refs.c | 5 +++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index a367220..f251acd 100644
--- a/cache.h
+++ b/cache.h
@@ -575,10 +575,10 @@ extern int refresh_index(struct index_state *, unsigned 
int flags, const struct
 #define LOCK_SUFFIX_LEN 5
 
 struct lock_file {
-   struct lock_file *next;
+   struct lock_file *volatile next;
volatile sig_atomic_t active;
-   int fd;
-   pid_t owner;
+   volatile int fd;
+   volatile pid_t owner;
char on_list;
char filename[PATH_MAX];
 };
diff --git a/lockfile.c b/lockfile.c
index 55fbb41..a09b4a3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -70,7 +70,7 @@
  * See Documentation/api-lockfile.txt for more information.
  */
 
-static struct lock_file *lock_file_list;
+static struct lock_file *volatile lock_file_list;
 
 static void remove_lock_file(void)
 {
diff --git a/refs.c b/refs.c
index 4f313bc..9971ac5 100644
--- a/refs.c
+++ b/refs.c
@@ -2259,15 +2259,16 @@ int commit_packed_refs(void)
get_packed_ref_cache(&ref_cache);
int error = 0;
int save_errno = 0;
+   int fd;
 
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
write_or_die(packed_ref_cache->lock->fd,
 PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 
+   fd = packed_ref_cache->lock->fd;
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
-0, write_packed_entry_fn,
-&packed_ref_cache->lock->fd);
+0, write_packed_entry_fn, &fd);
if (commit_lock_file(packed_ref_cache->lock)) {
save_errno = errno;
error = -1;
-- 
2.1.0

--
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 v5 27/35] commit_lock_file(): use a strbuf to manage temporary space

[Michael Haggerty]

Avoid relying on the filename length restrictions that are currently
checked by lock_file().

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index a09b4a3..59c822d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -325,8 +325,9 @@ int reopen_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-   char result_file[PATH_MAX];
+   static struct strbuf result_file = STRBUF_INIT;
int save_errno;
+   int err;
 
if (!lk->active)
die("BUG: attempt to commit unlocked object");
@@ -334,13 +335,13 @@ int commit_lock_file(struct lock_file *lk)
if (lk->fd >= 0 && close_lock_file(lk))
goto rollback;
 
-   strcpy(result_file, lk->filename);
/* remove ".lock": */
-   result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
-
-   if (rename(lk->filename, result_file))
+   strbuf_add(&result_file, lk->filename,
+  strlen(lk->filename) - LOCK_SUFFIX_LEN);
+   err = rename(lk->filename, result_file.buf);
+   strbuf_reset(&result_file);
+   if (err)
goto rollback;
-
lk->active = 0;
lk->filename[0] = 0;
return 0;
-- 
2.1.0

--
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 v5 34/35] lockfile.c: rename static functions

[Michael Haggerty]

* remove_lock_file() -> remove_lock_files()
* remove_lock_file_on_signal() -> remove_lock_files_on_signal()

Suggested-by: Torsten Bögershausen 
Signed-off-by: Michael Haggerty 
---
 lockfile.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 432d624..0b63554 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -73,7 +73,7 @@
 
 static struct lock_file *volatile lock_file_list;
 
-static void remove_lock_file(void)
+static void remove_lock_files(void)
 {
pid_t me = getpid();
 
@@ -84,9 +84,9 @@ static void remove_lock_file(void)
}
 }
 
-static void remove_lock_file_on_signal(int signo)
+static void remove_lock_files_on_signal(int signo)
 {
-   remove_lock_file();
+   remove_lock_files();
sigchain_pop(signo);
raise(signo);
 }
@@ -162,8 +162,8 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
 
if (!lock_file_list) {
/* One-time initialization */
-   sigchain_push_common(remove_lock_file_on_signal);
-   atexit(remove_lock_file);
+   sigchain_push_common(remove_lock_files_on_signal);
+   atexit(remove_lock_files);
}
 
if (lk->active)
-- 
2.1.0

--
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 v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Michael Haggerty]

It used to be that if locking failed, lock_file() usually did not
register the lock_file object in lock_file_list but sometimes it did.
This confusion was compounded if lock_file() was called via
hold_lock_file_for_append(), which has its own failure modes.

The ambiguity didn't have any ill effects, because lock_file objects
cannot be removed from the lock_file_list anyway.  But it is
unnecessary to leave this behavior inconsistent.

So change lock_file() to *always* ensure that the lock_file object is
registered in lock_file_list regardless of whether an error occurs.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 983c3ec..00c972c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -129,6 +129,22 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
 */
static const size_t max_path_len = sizeof(lk->filename) - 5;
 
+   if (!lock_file_list) {
+   /* One-time initialization */
+   sigchain_push_common(remove_lock_file_on_signal);
+   atexit(remove_lock_file);
+   }
+
+   if (!lk->on_list) {
+   /* Initialize *lk and add it to lock_file_list: */
+   lk->fd = -1;
+   lk->owner = 0;
+   lk->on_list = 1;
+   lk->filename[0] = 0;
+   lk->next = lock_file_list;
+   lock_file_list = lk;
+   }
+
if (strlen(path) >= max_path_len) {
errno = ENAMETOOLONG;
return -1;
@@ -139,16 +155,7 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
strcat(lk->filename, ".lock");
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
if (0 <= lk->fd) {
-   if (!lock_file_list) {
-   sigchain_push_common(remove_lock_file_on_signal);
-   atexit(remove_lock_file);
-   }
lk->owner = getpid();
-   if (!lk->on_list) {
-   lk->next = lock_file_list;
-   lock_file_list = lk;
-   lk->on_list = 1;
-   }
if (adjust_shared_perm(lk->filename)) {
int save_errno = errno;
error("cannot fix permission bits on %s",
-- 
2.1.0

--
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 v5 09/35] lockfile.c: document the various states of lock_file objects

[Michael Haggerty]

Document the valid states of lock_file objects, how they get into each
state, and how the state is encoded in the object's fields.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 lockfile.c | 57 +
 1 file changed, 57 insertions(+)

diff --git a/lockfile.c b/lockfile.c
index 00c972c..b6fe848 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,63 @@
 #include "cache.h"
 #include "sigchain.h"
 
+/*
+ * File write-locks as used by Git.
+ *
+ * When a file at $FILENAME needs to be written, it is done as
+ * follows:
+ *
+ * 1. Obtain a lock on the file by creating a lockfile at path
+ *$FILENAME.lock.  The file is opened for read/write using O_CREAT
+ *and O_EXCL mode to ensure that it doesn't already exist.  Such a
+ *lock file is respected by writers *but not by readers*.
+ *
+ *Usually, if $FILENAME is a symlink, then it is resolved, and the
+ *file ultimately pointed to is the one that is locked and later
+ *replaced.  However, if LOCK_NODEREF is used, then $FILENAME
+ *itself is locked and later replaced, even if it is a symlink.
+ *
+ * 2. Write the new file contents to the lockfile.
+ *
+ * 3. Move the lockfile to its final destination using rename(2).
+ *
+ * Instead of (3), the change can be rolled back by deleting lockfile.
+ *
+ * This module keeps track of all locked files in lock_file_list.
+ * When the first file is locked, it registers an atexit(3) handler;
+ * when the program exits, the handler rolls back any files that have
+ * been locked but were never committed or rolled back.
+ *
+ * A lock_file is owned by the process that created it. The lock_file
+ * object has an "owner" field that records its owner. This field is
+ * used to prevent a forked process from closing a lock_file of its
+ * parent.
+ *
+ * A lock_file object can be in several states:
+ *
+ * - Uninitialized.  In this state the object's on_list field must be
+ *   zero but the rest of its contents need not be initialized.  As
+ *   soon as the object is used in any way, it is irrevocably
+ *   registered in the lock_file_list, and on_list is set.
+ *
+ * - Locked, lockfile open (after hold_lock_file_for_update(),
+ *   hold_lock_file_for_append(), or reopen_lock_file()). In this
+ *   state, the lockfile exists, filename holds the filename of the
+ *   lockfile, fd holds a file descriptor open for writing to the
+ *   lockfile, and owner holds the PID of the process that locked the
+ *   file.
+ *
+ * - Locked, lockfile closed (after close_lock_file()).  Same as the
+ *   previous state, except that the lockfile is closed and fd is -1.
+ *
+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
+ *   failed attempt to lock).  In this state, filename[0] == '\0' and
+ *   fd is -1.  The object is left registered in the lock_file_list,
+ *   and on_list is set.
+ *
+ * See Documentation/api-lockfile.txt for more information.
+ */
+
 static struct lock_file *lock_file_list;
 
 static void remove_lock_file(void)
-- 
2.1.0

--
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 v5 29/35] resolve_symlink(): use a strbuf for internal scratch space

[Michael Haggerty]

Aside from shortening and simplifying the code, this removes another
place where the path name length is arbitrarily limited.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 33 -
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index eecb526..ae55f33 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -141,44 +141,35 @@ static char *last_path_elm(char *p)
 static char *resolve_symlink(char *p, size_t s)
 {
int depth = MAXDEPTH;
+   static struct strbuf link = STRBUF_INIT;
 
while (depth--) {
-   char link[PATH_MAX];
-   int link_len = readlink(p, link, sizeof(link));
-   if (link_len < 0) {
-   /* not a symlink anymore */
-   return p;
-   }
-   else if (link_len < sizeof(link))
-   /* readlink() never null-terminates */
-   link[link_len] = '\0';
-   else {
-   warning("%s: symlink too long", p);
-   return p;
-   }
+   if (strbuf_readlink(&link, p, strlen(p)) < 0)
+   break;
 
-   if (is_absolute_path(link)) {
+   if (is_absolute_path(link.buf)) {
/* absolute path simply replaces p */
-   if (link_len < s)
-   strcpy(p, link);
+   if (link.len < s)
+   strcpy(p, link.buf);
else {
warning("%s: symlink too long", p);
-   return p;
+   break;
}
} else {
/*
-* link is a relative path, so I must replace the
+* link is a relative path, so replace the
 * last element of p with it.
 */
char *r = (char *)last_path_elm(p);
-   if (r - p + link_len < s)
-   strcpy(r, link);
+   if (r - p + link.len < s)
+   strcpy(r, link.buf);
else {
warning("%s: symlink too long", p);
-   return p;
+   break;
}
}
}
+   strbuf_reset(&link);
return p;
 }
 
-- 
2.1.0

--
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 v5 30/35] resolve_symlink(): take a strbuf parameter

[Michael Haggerty]

Change resolve_symlink() to take a strbuf rather than a string as
parameter.  This simplifies the code and removes an arbitrary pathname
length restriction.  It also means that lock_file's filename field no
longer needs to be initialized to a large size.

Helped-by: Torsten Bögershausen 
Signed-off-by: Michael Haggerty 
---
 lockfile.c | 57 ++---
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index ae55f33..e689a84 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -124,58 +124,47 @@ static char *last_path_elm(char *p)
 #define MAXDEPTH 5
 
 /*
- * p = path that may be a symlink
- * s = full size of p
+ * path contains a path that might be a symlink.
  *
- * If p is a symlink, attempt to overwrite p with a path to the real
- * file or directory (which may or may not exist), following a chain of
- * symlinks if necessary.  Otherwise, leave p unmodified.
+ * If path is a symlink, attempt to overwrite it with a path to the
+ * real file or directory (which may or may not exist), following a
+ * chain of symlinks if necessary.  Otherwise, leave path unmodified.
  *
- * This is a best-effort routine.  If an error occurs, p will either be
- * left unmodified or will name a different symlink in a symlink chain
- * that started with p's initial contents.
- *
- * Always returns p.
+ * This is a best-effort routine.  If an error occurs, path will
+ * either be left unmodified or will name a different symlink in a
+ * symlink chain that started with the original path.
  */
-
-static char *resolve_symlink(char *p, size_t s)
+static void resolve_symlink(struct strbuf *path)
 {
int depth = MAXDEPTH;
static struct strbuf link = STRBUF_INIT;
 
while (depth--) {
-   if (strbuf_readlink(&link, p, strlen(p)) < 0)
+   if (strbuf_readlink(&link, path->buf, path->len) < 0)
break;
 
-   if (is_absolute_path(link.buf)) {
+   if (is_absolute_path(link.buf))
/* absolute path simply replaces p */
-   if (link.len < s)
-   strcpy(p, link.buf);
-   else {
-   warning("%s: symlink too long", p);
-   break;
-   }
-   } else {
+   strbuf_reset(path);
+   else {
/*
 * link is a relative path, so replace the
 * last element of p with it.
 */
-   char *r = (char *)last_path_elm(p);
-   if (r - p + link.len < s)
-   strcpy(r, link.buf);
-   else {
-   warning("%s: symlink too long", p);
-   break;
-   }
+   char *r = last_path_elm(path->buf);
+   strbuf_setlen(path, r - path->buf);
}
+
+   strbuf_addbuf(path, &link);
}
strbuf_reset(&link);
-   return p;
 }
 
 /* Make sure errno contains a meaningful value on error */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
+   size_t pathlen = strlen(path);
+
if (!lock_file_list) {
/* One-time initialization */
sigchain_push_common(remove_lock_file_on_signal);
@@ -192,7 +181,7 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
lk->active = 0;
lk->owner = 0;
lk->on_list = 1;
-   strbuf_init(&lk->filename, PATH_MAX);
+   strbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);
lk->next = lock_file_list;
lock_file_list = lk;
} else if (lk->filename.len) {
@@ -201,11 +190,9 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
path);
}
 
-   strbuf_addstr(&lk->filename, path);
-   if (!(flags & LOCK_NODEREF)) {
-   resolve_symlink(lk->filename.buf, lk->filename.alloc);
-   strbuf_setlen(&lk->filename, strlen(lk->filename.buf));
-   }
+   strbuf_add(&lk->filename, path, pathlen);
+   if (!(flags & LOCK_NODEREF))
+   resolve_symlink(&lk->filename);
strbuf_addstr(&lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
if (lk->fd < 0) {
-- 
2.1.0

--
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 v5 20/35] api-lockfile: document edge cases

[Michael Haggerty]

* Document the behavior of commit_lock_file() when it fails, namely
  that it rolls back the lock_file object and sets errno
  appropriately.

* Document the behavior of rollback_lock_file() when called for a
  lock_file object that has already been committed or rolled back,
  namely that it is a NOOP.

Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index 9a94ead..2514559 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -64,19 +64,22 @@ unable_to_lock_die::
 
 commit_lock_file::
 
-   Take a pointer to the `struct lock_file` initialized
-   with an earlier call to `hold_lock_file_for_update()`,
-   close the file descriptor and rename the lockfile to its
-   final destination.  Returns 0 upon success, a negative
-   value on failure to close(2) or rename(2).  It is a bug to
-   call `commit_lock_file()` for a `lock_file` object that is not
+   Take a pointer to the `struct lock_file` initialized with an
+   earlier call to `hold_lock_file_for_update()`, close the file
+   descriptor and rename the lockfile to its final destination.
+   Return 0 upon success.  On failure, rollback the lock file and
+   return -1, with `errno` set to the value from the failing call
+   to `close(2)` or `rename(2)`.  It is a bug to call
+   `commit_lock_file()` for a `lock_file` object that is not
currently locked.
 
 rollback_lock_file::
 
Take a pointer to the `struct lock_file` initialized
with an earlier call to `hold_lock_file_for_update()`,
-   close the file descriptor and remove the lockfile.
+   close the file descriptor and remove the lockfile.  It is a
+   NOOP to call `rollback_lock_file()` for a `lock_file` object
+   that has already been committed or rolled back.
 
 close_lock_file::
Take a pointer to the `struct lock_file` initialized
-- 
2.1.0

--
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 v5 26/35] try_merge_strategy(): use a statically-allocated lock_file object

[Michael Haggerty]

Even the one lockfile object needn't be allocated each time the
function is called.  Instead, define one statically-allocated
lock_file object and reuse it for every call.

Suggested-by: Jeff King 
Signed-off-by: Michael Haggerty 
---
 builtin/merge.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 6b430f0..1a5a5f2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -656,14 +656,14 @@ static int try_merge_strategy(const char *strategy, 
struct commit_list *common,
  struct commit_list *remoteheads,
  struct commit *head, const char *head_arg)
 {
-   struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+   static struct lock_file lock;
 
-   hold_locked_index(lock, 1);
+   hold_locked_index(&lock, 1);
refresh_cache(REFRESH_QUIET);
if (active_cache_changed &&
-   write_locked_index(&the_index, lock, COMMIT_LOCK))
+   write_locked_index(&the_index, &lock, COMMIT_LOCK))
return error(_("Unable to write index."));
-   rollback_lock_file(lock);
+   rollback_lock_file(&lock);
 
if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
int clean, x;
@@ -695,13 +695,13 @@ static int try_merge_strategy(const char *strategy, 
struct commit_list *common,
for (j = common; j; j = j->next)
commit_list_insert(j->item, &reversed);
 
-   hold_locked_index(lock, 1);
+   hold_locked_index(&lock, 1);
clean = merge_recursive(&o, head,
remoteheads->item, reversed, &result);
if (active_cache_changed &&
-   write_locked_index(&the_index, lock, COMMIT_LOCK))
+   write_locked_index(&the_index, &lock, COMMIT_LOCK))
die (_("unable to write %s"), get_index_file());
-   rollback_lock_file(lock);
+   rollback_lock_file(&lock);
return clean ? 0 : 1;
} else {
return try_merge_command(strategy, xopts_nr, xopts,
-- 
2.1.0

--
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 v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted

[Michael Haggerty]

If the call to adjust_shared_perm() fails, lock_file returns -1, which
to the caller looks like any other failure to lock the file.  So in
this case, roll back the lockfile before returning so that the lock
file is deleted immediately and the lockfile object is left in a
predictable state (namely, unlocked).  Previously, the lockfile was
retained until process cleanup in this situation.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lockfile.c b/lockfile.c
index b1c4ba0..d4f165d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
int save_errno = errno;
error("cannot fix permission bits on %s",
  lk->filename);
+   rollback_lock_file(lk);
errno = save_errno;
return -1;
}
-- 
2.1.0

--
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 v5 28/35] Change lock_file::filename into a strbuf

[Michael Haggerty]

For now, we still make sure to allocate at least PATH_MAX characters
for the strbuf because resolve_symlink() doesn't know how to expand
the space for its return value.  (That will be fixed in a moment.)

Another alternative would be to just use a strbuf as scratch space in
lock_file() but then store a pointer to the naked string in struct
lock_file.  But lock_file objects are often reused.  By reusing the
same strbuf, we can avoid having to reallocate the string most times
when a lock_file object is reused.

Helped-by: Torsten Bögershausen 
Signed-off-by: Michael Haggerty 
---
 builtin/commit.c | 12 ++--
 builtin/reflog.c |  2 +-
 cache.h  |  2 +-
 config.c | 14 +++---
 lockfile.c   | 51 +++
 read-cache.c |  4 ++--
 refs.c   |  6 +++---
 shallow.c|  6 +++---
 8 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d4228c9..4049d06 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -341,7 +341,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
die(_("unable to create temporary index"));
 
old_index_env = getenv(INDEX_ENVIRONMENT);
-   setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
+   setenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1);
 
if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
die(_("interactive add failed"));
@@ -352,7 +352,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
unsetenv(INDEX_ENVIRONMENT);
 
discard_cache();
-   read_cache_from(index_lock.filename);
+   read_cache_from(index_lock.filename.buf);
if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
if (reopen_lock_file(&index_lock) < 0)
die(_("unable to write index file"));
@@ -362,7 +362,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
warning(_("Failed to update main cache tree"));
 
commit_style = COMMIT_NORMAL;
-   return index_lock.filename;
+   return index_lock.filename.buf;
}
 
/*
@@ -385,7 +385,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
die(_("unable to write new_index file"));
commit_style = COMMIT_NORMAL;
-   return index_lock.filename;
+   return index_lock.filename.buf;
}
 
/*
@@ -472,9 +472,9 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
die(_("unable to write temporary index file"));
 
discard_cache();
-   read_cache_from(false_lock.filename);
+   read_cache_from(false_lock.filename.buf);
 
-   return false_lock.filename;
+   return false_lock.filename.buf;
 }
 
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
diff --git a/builtin/reflog.c b/builtin/reflog.c
index e8a8fb1..7c78b15 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 write_str_in_full(lock->lock_fd, "\n") != 1 ||
 close_ref(lock) < 0)) {
status |= error("Couldn't write %s",
-   lock->lk->filename);
+   lock->lk->filename.buf);
unlink(newlog_path);
} else if (rename(newlog_path, log_file)) {
status |= error("cannot rename %s to %s",
diff --git a/cache.h b/cache.h
index f251acd..307cc8e 100644
--- a/cache.h
+++ b/cache.h
@@ -580,7 +580,7 @@ struct lock_file {
volatile int fd;
volatile pid_t owner;
char on_list;
-   char filename[PATH_MAX];
+   struct strbuf filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index a8ab809..34c93c7 100644
--- a/config.c
+++ b/config.c
@@ -2017,9 +2017,9 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
 
-   if (chmod(lock->filename, st.st_mode & 0) < 0) {
+   if (chmod(lock->filename.buf, st.st_mode & 0) < 0) {
error("chmod on %s failed: %s",
-   lock->filename, strerror(errno));
+   lock->filename.buf, strerror(errno));
ret = CONFIG_NO_WRITE;
goto out_free;
}
@@ 

[PATCH v5 15/35] remove_lock_file(): call rollback_lock_file()

[Michael Haggerty]

It does just what we need.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 911f123..1c85b18 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -68,12 +68,8 @@ static void remove_lock_file(void)
pid_t me = getpid();
 
while (lock_file_list) {
-   if (lock_file_list->owner == me &&
-   lock_file_list->filename[0]) {
-   if (lock_file_list->fd >= 0)
-   close(lock_file_list->fd);
-   unlink_or_warn(lock_file_list->filename);
-   }
+   if (lock_file_list->owner == me)
+   rollback_lock_file(lock_file_list);
lock_file_list = lock_file_list->next;
}
 }
-- 
2.1.0

--
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 v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

It's bad manners. Especially since there could be a signal during the
call to unlink_or_warn(), in which case the signal handler will see
the wrong filename and delete the reference file, leaving the lockfile
behind.

So make our own copy to work with.

Signed-off-by: Michael Haggerty 
---
 refs.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 53cd4fb..11c76ec 100644
--- a/refs.c
+++ b/refs.c
@@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, 
struct strbuf *err)
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-   /* loose */
-   int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
-
-   lock->lk->filename[i] = 0;
-   err = unlink_or_warn(lock->lk->filename);
-   lock->lk->filename[i] = LOCK_SUFFIX[0];
+   /*
+* loose.  The loose file name is the same as the
+* lockfile name, minus ".lock":
+*/
+   char *loose_filename = xmemdupz(
+   lock->lk->filename,
+   strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);
+   int err = unlink_or_warn(loose_filename);
+   free(loose_filename);
if (err && errno != ENOENT)
return 1;
}
-- 
2.1.0

--
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 v5 03/35] rollback_lock_file(): do not clear filename redundantly

[Michael Haggerty]

It is only necessary to clear the lock_file's filename field if it was
not already clear.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 lockfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index f1ce154..a548e08 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -276,6 +276,6 @@ void rollback_lock_file(struct lock_file *lk)
if (lk->fd >= 0)
close(lk->fd);
unlink_or_warn(lk->filename);
+   lk->filename[0] = 0;
}
-   lk->filename[0] = 0;
 }
-- 
2.1.0

--
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 v5 19/35] commit_lock_file(): rollback lock file on failure to rename

[Michael Haggerty]

If rename() fails, call rollback_lock_file() to delete the lock file
(in case it is still present) and reset the filename field to the
empty string so that the lockfile object is left in a valid state.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index becb3da..6ae5c84 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -311,25 +311,29 @@ int reopen_lock_file(struct lock_file *lk)
 int commit_lock_file(struct lock_file *lk)
 {
char result_file[PATH_MAX];
+   int save_errno;
 
if (!lk->filename[0])
die("BUG: attempt to commit unlocked object");
 
-   if (lk->fd >= 0 && close_lock_file(lk)) {
-   int save_errno = errno;
-   rollback_lock_file(lk);
-   errno = save_errno;
-   return -1;
-   }
+   if (lk->fd >= 0 && close_lock_file(lk))
+   goto rollback;
 
strcpy(result_file, lk->filename);
/* remove ".lock": */
result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
 
if (rename(lk->filename, result_file))
-   return -1;
+   goto rollback;
+
lk->filename[0] = 0;
return 0;
+
+rollback:
+   save_errno = errno;
+   rollback_lock_file(lk);
+   errno = save_errno;
+   return -1;
 }
 
 int hold_locked_index(struct lock_file *lk, int die_on_error)
-- 
2.1.0

--
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 v5 16/35] commit_lock_file(): inline temporary variable

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 1c85b18..9e1cdd2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -311,12 +311,14 @@ int reopen_lock_file(struct lock_file *lk)
 int commit_lock_file(struct lock_file *lk)
 {
char result_file[PATH_MAX];
-   size_t i;
+
if (lk->fd >= 0 && close_lock_file(lk))
return -1;
+
strcpy(result_file, lk->filename);
-   i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
-   result_file[i] = 0;
+   /* remove ".lock": */
+   result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
+
if (rename(lk->filename, result_file))
return -1;
lk->filename[0] = 0;
-- 
2.1.0

--
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 v5 13/35] write_packed_entry_fn(): convert cb_data into a (const int *)

[Michael Haggerty]

This makes it obvious that we have no plans to change the integer
pointed to, which is actually the fd field from a struct lock_file.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 11c76ec..4f313bc 100644
--- a/refs.c
+++ b/refs.c
@@ -2217,7 +2217,7 @@ static void write_packed_entry(int fd, char *refname, 
unsigned char *sha1,
  */
 static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 {
-   int *fd = cb_data;
+   const int *fd = cb_data;
enum peel_status peel_status = peel_entry(entry, 0);
 
if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
-- 
2.1.0

--
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 v5 04/35] rollback_lock_file(): exit early if lock is not active

[Michael Haggerty]

Eliminate a layer of nesting.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 lockfile.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index a548e08..49179d8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -272,10 +272,11 @@ int hold_locked_index(struct lock_file *lk, int 
die_on_error)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-   if (lk->filename[0]) {
-   if (lk->fd >= 0)
-   close(lk->fd);
-   unlink_or_warn(lk->filename);
-   lk->filename[0] = 0;
-   }
+   if (!lk->filename[0])
+   return;
+
+   if (lk->fd >= 0)
+   close(lk->fd);
+   unlink_or_warn(lk->filename);
+   lk->filename[0] = 0;
 }
-- 
2.1.0

--
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 v5 14/35] lock_file(): exit early if lockfile cannot be opened

[Michael Haggerty]

This is a bit easier to read than the old version, which nested part
of the non-error code in an "if" block.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 lockfile.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 99a774b..911f123 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -212,19 +212,18 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
resolve_symlink(lk->filename, max_path_len);
strcat(lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
-   if (0 <= lk->fd) {
-   lk->owner = getpid();
-   if (adjust_shared_perm(lk->filename)) {
-   int save_errno = errno;
-   error("cannot fix permission bits on %s",
- lk->filename);
-   rollback_lock_file(lk);
-   errno = save_errno;
-   return -1;
-   }
-   }
-   else
+   if (lk->fd < 0) {
lk->filename[0] = 0;
+   return -1;
+   }
+   lk->owner = getpid();
+   if (adjust_shared_perm(lk->filename)) {
+   int save_errno = errno;
+   error("cannot fix permission bits on %s", lk->filename);
+   rollback_lock_file(lk);
+   errno = save_errno;
+   return -1;
+   }
return lk->fd;
 }
 
-- 
2.1.0

--
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 v5 02/35] api-lockfile: expand the documentation

[Michael Haggerty]

Document a couple more functions and the flags argument as used by
hold_lock_file_for_update() and hold_lock_file_for_append().

Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 36 +---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index dd89404..b53e300 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -28,9 +28,39 @@ hold_lock_file_for_update::
the final destination (e.g. `$GIT_DIR/index`) and a flag
`die_on_error`.  Attempt to create a lockfile for the
destination and return the file descriptor for writing
-   to the file.  If `die_on_error` flag is true, it dies if
-   a lock is already taken for the file; otherwise it
-   returns a negative integer to the caller on failure.
+   to the file.  The flags parameter is a combination of
++
+--
+LOCK_NODEREF::
+
+   Usually symbolic links in path are resolved in path and the
+   lockfile is created by adding ".lock" to the resolved path;
+   however, if `LOCK_NODEREF` is set, then the lockfile is
+   created by adding ".lock" to the path argument itself.
+
+LOCK_DIE_ON_ERROR::
+
+   If a lock is already taken for the file, `die()` with an error
+   message.  If this option is not specified, return a negative
+   integer to the caller on failure.
+--
+
+hold_lock_file_for_append::
+
+   Like `hold_lock_file_for_update()`, except that additionally
+   the existing contents of the file (if any) are copied to the
+   lockfile and its write pointer is positioned at the end of the
+   file before returning.
+
+unable_to_lock_error::
+
+   Emit an error describing that there was an error locking the
+   specified path.  The err parameter should be the errno of the
+   problem that caused the failure.
+
+unable_to_lock_die::
+
+   Like `unable_to_lock_error()`, but also `die()`.
 
 commit_lock_file::
 
-- 
2.1.0

--
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 v5 00/35] Lockfile correctness and refactoring

[Michael Haggerty]

Next iteration of my lockfile fixes and refactoring. Thanks to
Torsten Bögershausen, Junio, Peff, Ronnie Sahlberg, and Johannes Sixt
for their comments about v4.

I believe that this series addresses all of the comments from v1 [1],
v2 [2], v3 [3], and v4 [4].

Changes since v4:

* Rebase to current master.

* Explain lock_file ownership and the point of its pid field.

* Correct the log message for

"delete_ref_loose(): don't muck around in the lock_file's filename"

* Replace an assert() with a die("BUG:").

* Add a sanity check that lk->filename is empty before reusing a
  lock_file object.

* Initialize the length of lk->filename more intelligently.

* Rename trim_last_path_elm() to trim_last_path_component().

* Make some die() messages more informative.

* Add some sanity checks to commit_lock_file().

* Rename REF_NODEREF to REF_NO_DEREF.

* Rename some static functions:
  * remove_lock_file() -> remove_lock_files()
  * remove_lock_file_on_signal() -> remove_lock_files_on_signal()

* Add a function get_locked_file_path(), to isolate callers a bit more
  from the innards of lock_file.

There are some conflicts with branch rs/ref-transaction; I pushed my
proposed merge of these two branches to

https://github.com/mhagger/git.git lock-correctness-v5-rs-ref-transaction

[1] http://thread.gmane.org/gmane.comp.version-control.git/245609
[2] http://thread.gmane.org/gmane.comp.version-control.git/245801
[3] http://thread.gmane.org/gmane.comp.version-control.git/246222
[4] http://thread.gmane.org/gmane.comp.version-control.git/256564

Michael Haggerty (35):
  unable_to_lock_die(): rename function from unable_to_lock_index_die()
  api-lockfile: expand the documentation
  rollback_lock_file(): do not clear filename redundantly
  rollback_lock_file(): exit early if lock is not active
  rollback_lock_file(): set fd to -1
  lockfile: unlock file if lockfile permissions cannot be adjusted
  hold_lock_file_for_append(): release lock on errors
  lock_file(): always add lock_file object to lock_file_list
  lockfile.c: document the various states of lock_file objects
  cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
  delete_ref_loose(): don't muck around in the lock_file's filename
  prepare_index(): declare return value to be (const char *)
  write_packed_entry_fn(): convert cb_data into a (const int *)
  lock_file(): exit early if lockfile cannot be opened
  remove_lock_file(): call rollback_lock_file()
  commit_lock_file(): inline temporary variable
  commit_lock_file(): die() if called for unlocked lockfile object
  commit_lock_file(): if close fails, roll back
  commit_lock_file(): rollback lock file on failure to rename
  api-lockfile: document edge cases
  dump_marks(): remove a redundant call to rollback_lock_file()
  git_config_set_multivar_in_file(): avoid call to rollback_lock_file()
  lockfile: avoid transitory invalid states
  struct lock_file: declare some fields volatile
  try_merge_strategy(): remove redundant lock_file allocation
  try_merge_strategy(): use a statically-allocated lock_file object
  commit_lock_file(): use a strbuf to manage temporary space
  Change lock_file::filename into a strbuf
  resolve_symlink(): use a strbuf for internal scratch space
  resolve_symlink(): take a strbuf parameter
  trim_last_path_component(): replace last_path_elm()
  Extract a function commit_lock_file_to()
  Rename LOCK_NODEREF to LOCK_NO_DEREF
  lockfile.c: rename static functions
  get_locked_file_path(): new function

 Documentation/technical/api-lockfile.txt |  72 +--
 builtin/commit.c |  16 +-
 builtin/merge.c  |  15 +-
 builtin/reflog.c |   2 +-
 builtin/update-index.c   |   2 +-
 cache.h  |  19 +-
 config.c |  28 +--
 fast-import.c|   4 +-
 lockfile.c   | 334 +++
 read-cache.c |  12 +-
 refs.c   |  29 +--
 shallow.c|   6 +-
 12 files changed, 334 insertions(+), 205 deletions(-)

-- 
2.1.0

--
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 v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()

[Michael Haggerty]

This function is used for other things besides the index, so rename it
accordingly.

Suggested-by: Jeff King 
Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 builtin/update-index.c | 2 +-
 cache.h| 2 +-
 lockfile.c | 6 +++---
 refs.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index e8c7fd4..6c95988 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -942,7 +942,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
exit(128);
-   unable_to_lock_index_die(get_index_file(), lock_error);
+   unable_to_lock_die(get_index_file(), lock_error);
}
if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
die("Unable to write new index file");
diff --git a/cache.h b/cache.h
index dfa1a56..73ba5d0 100644
--- a/cache.h
+++ b/cache.h
@@ -582,7 +582,7 @@ struct lock_file {
 extern int unable_to_lock_error(const char *path, int err);
 extern void unable_to_lock_message(const char *path, int err,
   struct strbuf *buf);
-extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, 
int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, 
int);
 extern int commit_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 2a800ce..f1ce154 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err)
return -1;
 }
 
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int err)
 {
struct strbuf buf = STRBUF_INIT;
 
@@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const 
char *path, int flags)
 {
int fd = lock_file(lk, path, flags);
if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-   unable_to_lock_index_die(path, errno);
+   unable_to_lock_die(path, errno);
return fd;
 }
 
@@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const 
char *path, int flags)
fd = lock_file(lk, path, flags);
if (fd < 0) {
if (flags & LOCK_DIE_ON_ERROR)
-   unable_to_lock_index_die(path, errno);
+   unable_to_lock_die(path, errno);
return fd;
}
 
diff --git a/refs.c b/refs.c
index 2ce5d69..8cb3539 100644
--- a/refs.c
+++ b/refs.c
@@ -2167,7 +2167,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
 */
goto retry;
else
-   unable_to_lock_index_die(ref_file, errno);
+   unable_to_lock_die(ref_file, errno);
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
-- 
2.1.0

--
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 v5 35/35] get_locked_file_path(): new function

[Michael Haggerty]

Add a function to return the path of the file that is locked by a
lock_file object. This reduces the knowledge that callers have to have
about the lock_file layout.

Suggested-by: Ronnie Sahlberg 
Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 5 +
 cache.h  | 1 +
 lockfile.c   | 9 +
 refs.c   | 4 +---
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index 40cd524..22f1b5c 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -52,6 +52,11 @@ hold_lock_file_for_append::
lockfile and its write pointer is positioned at the end of the
file before returning.
 
+get_locked_file_path::
+
+   Return the path of the file that is locked by the specified
+   lock_file object. The caller must free the memory.
+
 unable_to_lock_error::
 
Emit an error describing that there was an error locking the
diff --git a/cache.h b/cache.h
index d610fab..4bbaffa 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int 
err,
 extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, 
int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, 
int);
+extern char *get_locked_file_path(struct lock_file *);
 extern int commit_lock_file_to(struct lock_file *, const char *path);
 extern int commit_lock_file(struct lock_file *);
 extern int reopen_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 0b63554..42cea0a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -274,6 +274,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const 
char *path, int flags)
return fd;
 }
 
+char *get_locked_file_path(struct lock_file *lk)
+{
+   if (!lk->active)
+   die("BUG: get_locked_file_path() called for unlocked object");
+   if (lk->filename.len <= LOCK_SUFFIX_LEN)
+   die("BUG: get_locked_file_path() called for malformed lock 
object");
+   return xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN);
+}
+
 int close_lock_file(struct lock_file *lk)
 {
int fd = lk->fd;
diff --git a/refs.c b/refs.c
index 525ce4b..5842dd0 100644
--- a/refs.c
+++ b/refs.c
@@ -2556,9 +2556,7 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag)
 * loose.  The loose file name is the same as the
 * lockfile name, minus ".lock":
 */
-   char *loose_filename = xmemdupz(
-   lock->lk->filename.buf,
-   lock->lk->filename.len - LOCK_SUFFIX_LEN);
+   char *loose_filename = get_locked_file_path(lock->lk);
int err = unlink_or_warn(loose_filename);
free(loose_filename);
if (err && errno != ENOENT)
-- 
2.1.0

--
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 v5 25/35] try_merge_strategy(): remove redundant lock_file allocation

[Michael Haggerty]

By the time the "if" block is entered, the lock_file instance from the
main function block is no longer in use, so re-use that one instead of
allocating a second one.

Note that the "lock" variable in the "if" block shadowed the "lock"
variable at function scope, so the only change needed is to remove the
inner definition.

Signed-off-by: Michael Haggerty 
---
 builtin/merge.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 9da9e30..6b430f0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -668,7 +668,6 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
int clean, x;
struct commit *result;
-   struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
struct commit_list *reversed = NULL;
struct merge_options o;
struct commit_list *j;
-- 
2.1.0

--
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 v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()

[Michael Haggerty]

When commit_lock_file() fails, it now always calls
rollback_lock_file() internally, so there is no need to call that
function here.

Signed-off-by: Michael Haggerty 
---
 fast-import.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index c071253..742d4d7 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1828,10 +1828,8 @@ static void dump_marks(void)
}
 
if (commit_lock_file(&mark_lock)) {
-   int saved_errno = errno;
-   rollback_lock_file(&mark_lock);
failure |= error("Unable to commit marks file %s: %s",
-   export_marks_file, strerror(saved_errno));
+   export_marks_file, strerror(errno));
return;
}
 }
-- 
2.1.0

--
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 v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object

[Michael Haggerty]

It was previously a bug to call commit_lock_file() with a lock_file
object that was not active (an illegal access would happen within the
function).  It was presumably never done, but this would be an easy
programming error to overlook.  So before continuing, do a consistency
check that the lock_file object really is locked.

Helped-by: Johannes Sixt 
Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 4 +++-
 lockfile.c   | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index b53e300..9a94ead 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -68,7 +68,9 @@ commit_lock_file::
with an earlier call to `hold_lock_file_for_update()`,
close the file descriptor and rename the lockfile to its
final destination.  Returns 0 upon success, a negative
-   value on failure to close(2) or rename(2).
+   value on failure to close(2) or rename(2).  It is a bug to
+   call `commit_lock_file()` for a `lock_file` object that is not
+   currently locked.
 
 rollback_lock_file::
 
diff --git a/lockfile.c b/lockfile.c
index 9e1cdd2..6436111 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -312,6 +312,9 @@ int commit_lock_file(struct lock_file *lk)
 {
char result_file[PATH_MAX];
 
+   if (!lk->filename[0])
+   die("BUG: attempt to commit unlocked object");
+
if (lk->fd >= 0 && close_lock_file(lk))
return -1;
 
-- 
2.1.0

--
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 v5 33/35] Rename LOCK_NODEREF to LOCK_NO_DEREF

[Michael Haggerty]

This makes it harder to misread the name as LOCK_NODE_REF.

Suggested-by: Torsten Bögershausen 
Signed-off-by: Michael Haggerty 
---
 Documentation/technical/api-lockfile.txt | 4 ++--
 cache.h  | 2 +-
 lockfile.c   | 4 ++--
 refs.c   | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index 3ee4299..40cd524 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -31,11 +31,11 @@ hold_lock_file_for_update::
to the file.  The flags parameter is a combination of
 +
 --
-LOCK_NODEREF::
+LOCK_NO_DEREF::
 
Usually symbolic links in path are resolved in path and the
lockfile is created by adding ".lock" to the resolved path;
-   however, if `LOCK_NODEREF` is set, then the lockfile is
+   however, if `LOCK_NO_DEREF` is set, then the lockfile is
created by adding ".lock" to the path argument itself.
 
 LOCK_DIE_ON_ERROR::
diff --git a/cache.h b/cache.h
index 45688d5..d610fab 100644
--- a/cache.h
+++ b/cache.h
@@ -583,7 +583,7 @@ struct lock_file {
struct strbuf filename;
 };
 #define LOCK_DIE_ON_ERROR 1
-#define LOCK_NODEREF 2
+#define LOCK_NO_DEREF 2
 extern int unable_to_lock_error(const char *path, int err);
 extern void unable_to_lock_message(const char *path, int err,
   struct strbuf *buf);
diff --git a/lockfile.c b/lockfile.c
index 480c2ba..432d624 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -17,7 +17,7 @@
  *
  *Usually, if $FILENAME is a symlink, then it is resolved, and the
  *file ultimately pointed to is the one that is locked and later
- *replaced.  However, if LOCK_NODEREF is used, then $FILENAME
+ *replaced.  However, if LOCK_NO_DEREF is used, then $FILENAME
  *itself is locked and later replaced, even if it is a symlink.
  *
  * 2. Write the new file contents to the lockfile.
@@ -186,7 +186,7 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
}
 
strbuf_add(&lk->filename, path, pathlen);
-   if (!(flags & LOCK_NODEREF))
+   if (!(flags & LOCK_NO_DEREF))
resolve_symlink(&lk->filename);
strbuf_addstr(&lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
diff --git a/refs.c b/refs.c
index c6e15f9a..525ce4b 100644
--- a/refs.c
+++ b/refs.c
@@ -2134,7 +2134,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
lflags = 0;
if (flags & REF_NODEREF) {
refname = orig_refname;
-   lflags |= LOCK_NODEREF;
+   lflags |= LOCK_NO_DEREF;
}
lock->ref_name = xstrdup(refname);
lock->orig_ref_name = xstrdup(orig_refname);
-- 
2.1.0

--
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 v5 23/35] lockfile: avoid transitory invalid states

[Michael Haggerty]

Because remove_lock_file() can be called any time by the signal
handler, it is important that any lock_file objects that are in the
lock_file_list are always in a valid state.  And since lock_file
objects are often reused (but are never removed from lock_file_list),
that means we have to be careful whenever mutating a lock_file object
to always keep it in a well-defined state.

This was formerly not the case, because part of the state was encoded
by setting lk->filename to the empty string vs. a valid filename.  It
is wrong to assume that this string can be updated atomically; for
example, even

strcpy(lk->filename, value)

is unsafe.  But the old code was even more reckless; for example,

strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
strcat(lk->filename, ".lock");

During the call to resolve_symlink(), lk->filename contained the name
of the file that was being locked, not the name of the lockfile.  If a
signal were raised during that interval, then the signal handler would
have deleted the valuable file!

We could probably continue to use the filename field to encode the
state by being careful to write characters 1..N-1 of the filename
first, and then overwrite the NUL at filename[0] with the first
character of the filename, but that would be awkward and error-prone.

So, instead of using the filename field to determine whether the
lock_file object is active, add a new field "lock_file::active" for
this purpose.  Be careful to set this field only when filename really
contains the name of a file that should be deleted on cleanup.

Helped-by: Johannes Sixt 
Signed-off-by: Michael Haggerty 
---
 cache.h  |  1 +
 lockfile.c   | 47 ---
 read-cache.c |  1 +
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index e4e7c56..a367220 100644
--- a/cache.h
+++ b/cache.h
@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int 
flags, const struct
 
 struct lock_file {
struct lock_file *next;
+   volatile sig_atomic_t active;
int fd;
pid_t owner;
char on_list;
diff --git a/lockfile.c b/lockfile.c
index 6ae5c84..55fbb41 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -27,16 +27,19 @@
  * Instead of (3), the change can be rolled back by deleting lockfile.
  *
  * This module keeps track of all locked files in lock_file_list.
- * When the first file is locked, it registers an atexit(3) handler;
- * when the program exits, the handler rolls back any files that have
- * been locked but were never committed or rolled back.
+ * When the first file is locked, it registers an atexit(3) handler
+ * and a signal handler; when the program exits, the handler rolls
+ * back any files that have been locked but were never committed or
+ * rolled back.
  *
  * A lock_file is owned by the process that created it. The lock_file
  * object has an "owner" field that records its owner. This field is
  * used to prevent a forked process from closing a lock_file of its
  * parent.
  *
- * A lock_file object can be in several states:
+ * Because the signal handler can be called at any time, a lock_file
+ * object must always be in a well-defined state.  The possible states
+ * are as follows:
  *
  * - Uninitialized.  In this state the object's on_list field must be
  *   zero but the rest of its contents need not be initialized.  As
@@ -44,19 +47,25 @@
  *   registered in the lock_file_list, and on_list is set.
  *
  * - Locked, lockfile open (after hold_lock_file_for_update(),
- *   hold_lock_file_for_append(), or reopen_lock_file()). In this
- *   state, the lockfile exists, filename holds the filename of the
- *   lockfile, fd holds a file descriptor open for writing to the
- *   lockfile, and owner holds the PID of the process that locked the
- *   file.
+ *   hold_lock_file_for_append(), or reopen_lock_file()).  In this
+ *   state:
+ *   - the lockfile exists
+ *   - active is set
+ *   - filename holds the filename of the lockfile
+ *   - fd holds a file descriptor open for writing to the lockfile
+ *   - owner holds the PID of the process that locked the file
  *
  * - Locked, lockfile closed (after close_lock_file()).  Same as the
  *   previous state, except that the lockfile is closed and fd is -1.
  *
  * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
- *   failed attempt to lock).  In this state, filename[0] == '\0' and
- *   fd is -1.  The object is left registered in the lock_file_list,
- *   and on_list is set.
+ *   failed attempt to lock).  In this state:
+ *   - active is unset
+ *   - filename[0] == '\0' (usually, though there are transitory states
+ * in which this condition doesn't hold)
+ *   - fd is -1
+ *   - the object is left registered in the lock_file_list, and
+ * on_list is set.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -189,9 +198,14 @@ s

[PATCH v5 31/35] trim_last_path_component(): replace last_path_elm()

[Michael Haggerty]

Rewrite last_path_elm() to take a strbuf parameter and to trim off the
last path name element in place rather than returning a pointer to the
beginning of the last path name element. This simplifies the function
a bit and makes it integrate better with its caller, which is now also
strbuf-based. Rename the function accordingly and a bit less tersely.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index e689a84..3d5ab4f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -91,32 +91,28 @@ static void remove_lock_file_on_signal(int signo)
 }
 
 /*
- * p = absolute or relative path name
+ * path = absolute or relative path name
  *
- * Return a pointer into p showing the beginning of the last path name
- * element.  If p is empty or the root directory ("/"), just return p.
+ * Remove the last path name element from path (leaving the preceding
+ * "/", if any).  If path is empty or the root directory ("/"), set
+ * path to the empty string.
  */
-static char *last_path_elm(char *p)
+static void trim_last_path_component(struct strbuf *path)
 {
-   /* r starts pointing to null at the end of the string */
-   char *r = strchr(p, '\0');
-
-   if (r == p)
-   return p; /* just return empty string */
-
-   r--; /* back up to last non-null character */
+   int i = path->len;
 
/* back up past trailing slashes, if any */
-   while (r > p && *r == '/')
-   r--;
+   while (i && path->buf[i - 1] == '/')
+   i--;
 
/*
-* then go backwards until I hit a slash, or the beginning of
-* the string
+* then go backwards until a slash, or the beginning of the
+* string
 */
-   while (r > p && *(r-1) != '/')
-   r--;
-   return r;
+   while (i && path->buf[i - 1] != '/')
+   i--;
+
+   strbuf_setlen(path, i);
 }
 
 
@@ -146,14 +142,12 @@ static void resolve_symlink(struct strbuf *path)
if (is_absolute_path(link.buf))
/* absolute path simply replaces p */
strbuf_reset(path);
-   else {
+   else
/*
 * link is a relative path, so replace the
 * last element of p with it.
 */
-   char *r = last_path_elm(path->buf);
-   strbuf_setlen(path, r - path->buf);
-   }
+   trim_last_path_component(path);
 
strbuf_addbuf(path, &link);
}
-- 
2.1.0

--
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 v5 12/35] prepare_index(): declare return value to be (const char *)

[Michael Haggerty]

Declare the return value to be const to make it clear that we aren't
giving callers permission to write over the string that it points at.
(The return value is the filename field of a struct lock_file, which
can be used by a signal handler at any time and therefore shouldn't be
tampered with.)

Signed-off-by: Michael Haggerty 
---
 builtin/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 41f481b..d4228c9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -315,8 +315,8 @@ static void refresh_cache_or_die(int refresh_flags)
die_resolve_conflict("commit");
 }
 
-static char *prepare_index(int argc, const char **argv, const char *prefix,
-  const struct commit *current_head, int is_status)
+static const char *prepare_index(int argc, const char **argv, const char 
*prefix,
+const struct commit *current_head, int 
is_status)
 {
struct string_list partial;
struct pathspec pathspec;
-- 
2.1.0

--
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 v5 18/35] commit_lock_file(): if close fails, roll back

[Michael Haggerty]

If closing an open lockfile fails, then we cannot be sure of the
contents of the lockfile, so there is nothing sensible to do but
delete it.  This change also leaves the lock_file object in a defined
state in this error path (namely, unlocked).

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 6436111..becb3da 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -315,8 +315,12 @@ int commit_lock_file(struct lock_file *lk)
if (!lk->filename[0])
die("BUG: attempt to commit unlocked object");
 
-   if (lk->fd >= 0 && close_lock_file(lk))
+   if (lk->fd >= 0 && close_lock_file(lk)) {
+   int save_errno = errno;
+   rollback_lock_file(lk);
+   errno = save_errno;
return -1;
+   }
 
strcpy(result_file, lk->filename);
/* remove ".lock": */
-- 
2.1.0

--
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 v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()

[Michael Haggerty]

After commit_lock_file() is called, then the lock_file object is
necessarily either committed or rolled back.  So there is no need to
call rollback_lock_file() again in either of these cases.

Signed-off-by: Michael Haggerty 
---
 config.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 83c913a..a8ab809 100644
--- a/config.c
+++ b/config.c
@@ -2076,17 +2076,17 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
if (commit_lock_file(lock) < 0) {
error("could not commit config file %s", config_filename);
ret = CONFIG_NO_WRITE;
-   goto out_free;
-   }
+   } else
+   ret = 0;
 
/*
-* lock is committed, so don't try to roll it back below.
-* NOTE: Since lockfile.c keeps a linked list of all created
-* lock_file structures, it isn't safe to free(lock).  It's
-* better to just leave it hanging around.
+* lock is committed or rolled back now, so there is no need
+* to roll it back below.  NOTE: Since lockfile.c keeps a
+* linked list of all created lock_file structures, it isn't
+* safe to free(lock).  We have to just leave it hanging
+* around.
 */
lock = NULL;
-   ret = 0;
 
/* Invalidate the config cache */
git_config_clear();
-- 
2.1.0

--
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 v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty]

There are a few places that use these values, so define constants for
them.

Signed-off-by: Michael Haggerty 
---
 cache.h|  4 
 lockfile.c | 11 ++-
 refs.c |  7 ---
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 73ba5d0..e4e7c56 100644
--- a/cache.h
+++ b/cache.h
@@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, 
struct stat *st);
 #define REFRESH_IN_PORCELAIN   0x0020  /* user friendly output, not "needs 
update" */
 extern int refresh_index(struct index_state *, unsigned int flags, const 
struct pathspec *pathspec, char *seen, const char *header_msg);
 
+/* String appended to a filename to derive the lockfile name: */
+#define LOCK_SUFFIX ".lock"
+#define LOCK_SUFFIX_LEN 5
+
 struct lock_file {
struct lock_file *next;
int fd;
diff --git a/lockfile.c b/lockfile.c
index b6fe848..99a774b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -181,10 +181,11 @@ static char *resolve_symlink(char *p, size_t s)
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
/*
-* subtract 5 from size to make sure there's room for adding
-* ".lock" for the lock file name
+* subtract LOCK_SUFFIX_LEN from size to make sure there's
+* room for adding ".lock" for the lock file name:
 */
-   static const size_t max_path_len = sizeof(lk->filename) - 5;
+   static const size_t max_path_len = sizeof(lk->filename) -
+  LOCK_SUFFIX_LEN;
 
if (!lock_file_list) {
/* One-time initialization */
@@ -209,7 +210,7 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
-   strcat(lk->filename, ".lock");
+   strcat(lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
if (0 <= lk->fd) {
lk->owner = getpid();
@@ -319,7 +320,7 @@ int commit_lock_file(struct lock_file *lk)
if (lk->fd >= 0 && close_lock_file(lk))
return -1;
strcpy(result_file, lk->filename);
-   i = strlen(result_file) - 5; /* .lock */
+   i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
result_file[i] = 0;
if (rename(lk->filename, result_file))
return -1;
diff --git a/refs.c b/refs.c
index 8cb3539..53cd4fb 100644
--- a/refs.c
+++ b/refs.c
@@ -79,7 +79,8 @@ out:
if (refname[1] == '\0')
return -1; /* Component equals ".". */
}
-   if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
+   if (cp - refname >= LOCK_SUFFIX_LEN &&
+   !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
return -1; /* Refname ends with ".lock". */
return cp - refname;
 }
@@ -2551,11 +2552,11 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag)
 {
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
-   int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+   int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
 
lock->lk->filename[i] = 0;
err = unlink_or_warn(lock->lk->filename);
-   lock->lk->filename[i] = '.';
+   lock->lk->filename[i] = LOCK_SUFFIX[0];
if (err && errno != ENOENT)
return 1;
}
-- 
2.1.0

--
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 v5 05/35] rollback_lock_file(): set fd to -1

[Michael Haggerty]

When rolling back the lockfile, call close_lock_file() so that the
lock_file's fd field gets set back to -1.  This keeps the lock_file
object in a valid state, which is important because these objects are
allowed to be reused.

Signed-off-by: Michael Haggerty 
Reviewed-by: Ronnie Sahlberg 
---
 lockfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 49179d8..b1c4ba0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)
return;
 
if (lk->fd >= 0)
-   close(lk->fd);
+   close_lock_file(lk);
unlink_or_warn(lk->filename);
lk->filename[0] = 0;
 }
-- 
2.1.0

--
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 v5 07/35] hold_lock_file_for_append(): release lock on errors

[Michael Haggerty]

If there is an error copying the old contents to the lockfile, roll
back the lockfile before exiting so that the lockfile is not held
until process cleanup.

Signed-off-by: Michael Haggerty 
---
 lockfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index d4f165d..983c3ec 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const 
char *path, int flags)
if (errno != ENOENT) {
if (flags & LOCK_DIE_ON_ERROR)
die("cannot open '%s' for copying", path);
-   close(fd);
+   rollback_lock_file(lk);
return error("cannot open '%s' for copying", path);
}
} else if (copy_fd(orig_fd, fd)) {
if (flags & LOCK_DIE_ON_ERROR)
exit(128);
-   close(fd);
+   rollback_lock_file(lk);
return -1;
}
return fd;
-- 
2.1.0

--
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 1/2] add macro REALLOCARRAY

[Junio C Hamano]

René Scharfe  writes:

> Am 16.09.2014 um 05:04 schrieb Junio C Hamano:
>> On Sun, Sep 14, 2014 at 9:55 AM, René Scharfe  wrote:
>>> +#define REALLOCARRAY(x, alloc) x = xrealloc((x), (alloc) * sizeof(*(x)))
>>
>> I have been wondering if "x" could be an expression that has an operator
>> that binds weaker than the assignment '='.  That may necessitate the LHS
>> of the assignment to be somehow marked as bound the tightest, i.e.
>>
>> #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
>>
>> Or am I being overly silly?
>
> ALLOC_GROW did well without that.  I can't think of a good use case
> for a complex expression on the right-hand side.  That said, I think I
> still have a spare matching pair of parentheses lying around here
> somewhere, so let's play it safe and use them. :)

Yeah, as long as (any) is still an lvalue for any lvalue for
everybody's compiler, (x) = ... expression ... is safer, but
taking cue from ALLOC_GROW(), I would say I was probably being
slightly overly silly.


--
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/2] use REALLOC_ARRAY for changing the allocation size of arrays

[René Scharfe]

Signed-off-by: Rene Scharfe 
---
 attr.c |  3 +--
 builtin/apply.c|  2 +-
 builtin/for-each-ref.c |  9 +++--
 builtin/index-pack.c   |  4 +---
 builtin/log.c  |  2 +-
 builtin/merge.c|  2 +-
 builtin/mv.c   |  8 
 builtin/pack-objects.c |  3 +--
 builtin/show-branch.c  |  2 +-
 cache.h|  2 +-
 column.c   |  6 ++
 commit-slab.h  |  3 +--
 fast-import.c  |  2 +-
 git.c  |  3 +--
 graph.c| 14 --
 khash.h| 12 
 line-log.c |  2 +-
 object.c   |  2 +-
 pack-bitmap-write.c|  3 +--
 pack-bitmap.c  |  6 ++
 pack-objects.c |  3 +--
 revision.c |  2 +-
 sh-i18n--envsubst.c|  5 +
 shallow.c  |  3 +--
 string-list.c  |  3 +--
 walker.c   |  4 ++--
 26 files changed, 40 insertions(+), 70 deletions(-)

diff --git a/attr.c b/attr.c
index 734222d..cd54697 100644
--- a/attr.c
+++ b/attr.c
@@ -97,8 +97,7 @@ static struct git_attr *git_attr_internal(const char *name, 
int len)
a->attr_nr = attr_nr++;
git_attr_hash[pos] = a;
 
-   check_all_attr = xrealloc(check_all_attr,
- sizeof(*check_all_attr) * attr_nr);
+   REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
return a;
diff --git a/builtin/apply.c b/builtin/apply.c
index f204cca..8714a88 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2626,7 +2626,7 @@ static void update_image(struct image *img,
 * NOTE: this knows that we never call remove_first_line()
 * on anything other than pre/post image.
 */
-   img->line = xrealloc(img->line, nr * sizeof(*img->line));
+   REALLOC_ARRAY(img->line, nr);
img->line_allocated = img->line;
}
if (preimage_limit != postimage->nr)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 47bd624..7f55e68 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -138,10 +138,8 @@ static int parse_atom(const char *atom, const char *ep)
/* Add it in, including the deref prefix */
at = used_atom_cnt;
used_atom_cnt++;
-   used_atom = xrealloc(used_atom,
-(sizeof *used_atom) * used_atom_cnt);
-   used_atom_type = xrealloc(used_atom_type,
- (sizeof(*used_atom_type) * used_atom_cnt));
+   REALLOC_ARRAY(used_atom, used_atom_cnt);
+   REALLOC_ARRAY(used_atom_type, used_atom_cnt);
used_atom[at] = xmemdupz(atom, ep - atom);
used_atom_type[at] = valid_atom[i].cmp_type;
if (*atom == '*')
@@ -870,8 +868,7 @@ static int grab_single_ref(const char *refname, const 
unsigned char *sha1, int f
ref->flag = flag;
 
cnt = cb->grab_cnt;
-   cb->grab_array = xrealloc(cb->grab_array,
- sizeof(*cb->grab_array) * (cnt + 1));
+   REALLOC_ARRAY(cb->grab_array, cnt + 1);
cb->grab_array[cnt++] = ref;
cb->grab_cnt = cnt;
return 0;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..783623d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1140,9 +1140,7 @@ static void conclude_pack(int fix_thin_pack, const char 
*curr_pack, unsigned cha
int nr_objects_initial = nr_objects;
if (nr_unresolved <= 0)
die(_("confusion beyond insanity"));
-   objects = xrealloc(objects,
-  (nr_objects + nr_unresolved + 1)
-  * sizeof(*objects));
+   REALLOC_ARRAY(objects, nr_objects + nr_unresolved + 1);
memset(objects + nr_objects + 1, 0,
   nr_unresolved * sizeof(*objects));
f = sha1fd(output_fd, curr_pack);
diff --git a/builtin/log.c b/builtin/log.c
index e4d8122..7643396 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1440,7 +1440,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
continue;
 
nr++;
-   list = xrealloc(list, nr * sizeof(list[0]));
+   REALLOC_ARRAY(list, nr);
list[nr - 1] = commit;
}
if (nr == 0)
diff --git a/builtin/merge.c b/builtin/merge.c
index 9da9e30..cb9af1e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -556,7 +556,7 @@ static void parse_branch_merge_options(char *bmo)
if (argc < 0)
die(_("Bad branch.%s.mergeoptions string: %s"), branch,
split_cmdline_strerror(argc));
-   argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
+   REALLOC_ARRAY(argv, argc + 2);
memmove(argv + 1, argv, sizeof(*argv) * (ar

[PATCH 1/2] add macro REALLOC_ARRAY

[René Scharfe]

The macro ALLOC_GROW manages several aspects of dynamic memory
allocations for arrays: It performs overprovisioning in order to avoid
reallocations in future calls, updates the allocation size variable,
multiplies the item size and thus allows users to simply specify the
item count, performs the reallocation and updates the array pointer.

Sometimes this is too much.  Add the macro REALLOC_ARRAY, which only
takes care of the latter three points and allows users to specfiy the
number of items the array can store.  It can increase and also decrease
the size.  Using the macro avoid duplicating the variable name and
takes care of the item sizes automatically.

Signed-off-by: Rene Scharfe 
---
 Documentation/technical/api-allocation-growing.txt | 3 +++
 git-compat-util.h  | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/technical/api-allocation-growing.txt 
b/Documentation/technical/api-allocation-growing.txt
index 542946b..5a59b54 100644
--- a/Documentation/technical/api-allocation-growing.txt
+++ b/Documentation/technical/api-allocation-growing.txt
@@ -34,3 +34,6 @@ item[nr++] = value you like;
 
 
 You are responsible for updating the `nr` variable.
+
+If you need to specify the number of elements to allocate explicitly
+then use the macro `REALLOC_ARRAY(item, alloc)` instead of `ALLOC_GROW`.
diff --git a/git-compat-util.h b/git-compat-util.h
index 4e7e3f8..5a15b53 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -626,6 +626,8 @@ extern int odb_mkstemp(char *template, size_t limit, const 
char *pattern);
 extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
 extern char *xgetcwd(void);
 
+#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
+
 static inline size_t xsize_t(off_t len)
 {
if (len > (size_t) len)
-- 
2.1.0

--
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 1/2] add macro REALLOCARRAY

[René Scharfe]

Am 16.09.2014 um 05:04 schrieb Junio C Hamano:

On Sun, Sep 14, 2014 at 9:55 AM, René Scharfe  wrote:

+#define REALLOCARRAY(x, alloc) x = xrealloc((x), (alloc) * sizeof(*(x)))


I have been wondering if "x" could be an expression that has an operator
that binds weaker than the assignment '='.  That may necessitate the LHS
of the assignment to be somehow marked as bound the tightest, i.e.

#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))

Or am I being overly silly?


ALLOC_GROW did well without that.  I can't think of a good use case for 
a complex expression on the right-hand side.  That said, I think I still 
have a spare matching pair of parentheses lying around here somewhere, 
so let's play it safe and use them. :)


The added underscore is a good idea as well.

René
--
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] mailinfo: do not treat ">From" lines as in-body headers

[Junio C Hamano]

Junio C Hamano  writes:

>> I think you forgot to "git add" mbox.h. That being said, if we did go
>> this route, I do not see any reason to share the code at all. This can
>> be purely a mailinfo.c thing.
>
> OK.  A reroll coming today when I find time.

-- >8 --
From: Jeff King 
Date: Sat, 13 Sep 2014 21:30:38 -0400
Subject: [PATCH] mailinfo: make ">From" in-body header check more robust

Since commit 81c5cf7 (mailinfo: skip bogus UNIX From line inside
body, 2006-05-21), we have treated lines like ">From" in the body as
headers. This makes "git am" work for people who erroneously paste
the whole output from format-patch:

  From 12345abcd...fedcba543210 Mon Sep 17 00:00:00 2001
  From: them
  Subject: [PATCH] whatever

into their email body (assuming that an mbox writer then quotes
"From" as ">From", as otherwise we would actually mailsplit on the
in-body line).

However, this has false positives if somebody actually has a commit
body that starts with "From "; in this case we erroneously remove
the line entirely from the commit message. We can make this check
more robust by making sure the line actually looks like a real mbox
"From" line.

Inspect the line that begins with ">From " a more carefully to only
skip lines that match the expected pattern (note that the datestamp
part of the format-patch output is designed to be kept constant to
help those who write magic(5) entries).

Signed-off-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 builtin/mailinfo.c | 17 -
 t/t5100-mailinfo.sh| 18 ++
 t/t5100/embed-from.expect  |  5 +
 t/t5100/embed-from.in  | 13 +
 t/t5100/quoted-from.expect |  3 +++
 t/t5100/quoted-from.in | 10 ++
 6 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 t/t5100/embed-from.expect
 create mode 100644 t/t5100/embed-from.in
 create mode 100644 t/t5100/quoted-from.expect
 create mode 100644 t/t5100/quoted-from.in

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index cf11c8d..2632fb0 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -288,6 +288,21 @@ static inline int cmp_header(const struct strbuf *line, 
const char *hdr)
line->buf[len] == ':' && isspace(line->buf[len + 1]);
 }
 
+#define SAMPLE "From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 
00:00:00 2001\n"
+static int is_format_patch_separator(const char *line, int len)
+{
+   const char *cp;
+
+   if (len != strlen(SAMPLE))
+   return 0;
+   if (!skip_prefix(line, "From ", &cp))
+   return 0;
+   if (strspn(cp, "0123456789abcdef") != 40)
+   return 0;
+   cp += 40;
+   return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
+}
+
 static int check_header(const struct strbuf *line,
struct strbuf *hdr_data[], int overwrite)
 {
@@ -329,7 +344,7 @@ static int check_header(const struct strbuf *line,
 
/* for inbody stuff */
if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
-   ret = 1; /* Should this return 0? */
+   ret = is_format_patch_separator(line->buf + 1, line->len - 1);
goto check_header_out;
}
if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 3e64a7a..9e1ad1c 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -89,4 +89,22 @@ test_expect_success 'mailinfo on from header without name 
works' '
 
 '
 
+test_expect_success 'mailinfo finds headers after embedded From line' '
+   mkdir embed-from &&
+   git mailsplit -oembed-from "$TEST_DIRECTORY"/t5100/embed-from.in &&
+   test_cmp "$TEST_DIRECTORY"/t5100/embed-from.in embed-from/0001 &&
+   git mailinfo embed-from/msg embed-from/patch \
+ embed-from/out &&
+   test_cmp "$TEST_DIRECTORY"/t5100/embed-from.expect embed-from/out
+'
+
+test_expect_success 'mailinfo on message with quoted >From' '
+   mkdir quoted-from &&
+   git mailsplit -oquoted-from "$TEST_DIRECTORY"/t5100/quoted-from.in &&
+   test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.in quoted-from/0001 &&
+   git mailinfo quoted-from/msg quoted-from/patch \
+ quoted-from/out &&
+   test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
+'
+
 test_done
diff --git a/t/t5100/embed-from.expect b/t/t5100/embed-from.expect
new file mode 100644
index 000..06a3a38
--- /dev/null
+++ b/t/t5100/embed-from.expect
@@ -0,0 +1,5 @@
+Author: Commit Author
+Email: com...@example.com
+Subject: patch subject
+Date: Sat, 13 Sep 2014 21:13:23 -0400 
+
diff --git a/t/t5100/embed-from.in b/t/t5100/embed-from.in
new file mode 100644
index 000..5f3f84e
--- /dev/null
+++ b/t/t5100/embed-from.in
@@ -0,0 +1,13 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: Email Author 
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH

Re: [PATCH v5 00/23] Signed push

[Junio C Hamano]

Junio C Hamano  writes:

> A failing test has been added at the end for smart HTTP.  It appears
> that somewhere in the callchain "--signed" is forgotten and the
> sending end not to send the certificate for some reason.  If
> somebody with a fresh set of eyes can look into it, that would be
> very much appreciated, as I do not expect I would have sufficient
> concentration to dig it quickly for several days at least.

I lied.  This is to replace the patch at the end (23/23).

-- >8 --
From: Junio C Hamano 
Date: Mon, 15 Sep 2014 14:59:00 -0700
Subject: [PATCH] push: teach smart-HTTP to pass "git push --signed" around

The "--signed" option received by "git push" is first passed to the
transport layer, which the native transport directly uses to notice
that a push certificate needs to be sent.  When the transport-helper
is involved, however, the option needs to be told to the helper with
set_helper_option(), and the helepr needs to take necessary action.
For the smart-HTTP helper, the "necessary action" involves spawning
the "git send-pack" subprocess with the "--signed" option.

Once the above all gets wired in, the smart-HTTP transport now can
use the push certificate mechanism to authenticate its pushes.

Add a test that is modeled after tests for the native transport in
t5534-push-signed.sh to t5541-http-push-smart.sh.  Update the test
Apache configuration to pass GNUPGHOME environment variable through.

Signed-off-by: Junio C Hamano 
---

 * Unconditional addition of "PassEnv GNUPGHOME" may give warnings
   from Apache in other tests that do not do GPG, but I left it
   as-is because I didn't think of a good way to squelch it.
   Perhaps define it as an empty in test-lib.sh, to be overriden by
   lib-gpg.sh?

 builtin/send-pack.c|  4 
 remote-curl.c  | 13 -
 t/lib-httpd/apache.conf|  1 +
 t/t5541-http-push-smart.sh | 38 ++
 transport-helper.c |  9 -
 5 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..ca28d8d 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -153,6 +153,10 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.verbose = 1;
continue;
}
+   if (!strcmp(arg, "--signed")) {
+   args.push_cert = 1;
+   continue;
+   }
if (!strcmp(arg, "--progress")) {
progress = 1;
continue;
diff --git a/remote-curl.c b/remote-curl.c
index 0fcf2ce..1ea4e95 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -25,7 +25,8 @@ struct options {
update_shallow : 1,
followtags : 1,
dry_run : 1,
-   thin : 1;
+   thin : 1,
+   push_cert : 1;
 };
 static struct options options;
 static struct string_list cas_options = STRING_LIST_INIT_DUP;
@@ -106,6 +107,14 @@ static int set_option(const char *name, const char *value)
else
return -1;
return 0;
+   } else if (!strcmp(name, "pushcert")) {
+   if (!strcmp(value, "true"))
+   options.push_cert = 1;
+   else if (!strcmp(value, "false"))
+   options.push_cert = 0;
+   else
+   return -1;
+   return 0;
} else {
return 1 /* unsupported */;
}
@@ -872,6 +881,8 @@ static int push_git(struct discovery *heads, int nr_spec, 
char **specs)
argv_array_push(&args, "--thin");
if (options.dry_run)
argv_array_push(&args, "--dry-run");
+   if (options.push_cert)
+   argv_array_push(&args, "--signed");
if (options.verbosity == 0)
argv_array_push(&args, "--quiet");
else if (options.verbosity > 1)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index b384d79..7713dd2 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -68,6 +68,7 @@ LockFile accept.lock
 
 PassEnv GIT_VALGRIND
 PassEnv GIT_VALGRIND_OPTIONS
+PassEnv GNUPGHOME
 
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 73af16f..6552366 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -12,6 +12,7 @@ if test -n "$NO_CURL"; then
 fi
 
 ROOT_PATH="$PWD"
+. "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 start_httpd
@@ -323,5 +324,42 @@ test_expect_success 'push into half-auth-complete requires 
password' '
test_cmp expect actual
 '
 
+test_expect_success GPG 'push with post-receive to inspect certificate' '
+   (
+   

Re: [PATCH v3] pretty: add %D format specifier

[Junio C Hamano]

Harry Jeffery  writes:

> Add a new format specifier, '%D' that is identical in behaviour to '%d',
> except that it does not include the ' (' prefix or ')' suffix provided
> by '%d'.
>
> Signed-off-by: Harry Jeffery 

Thanks.

> @@ -196,20 +198,20 @@ void format_decorations(struct strbuf *sb,
>   decoration = lookup_decoration(&name_decoration, &commit->object);
>   if (!decoration)
>   return;
> - prefix = " (";
> + strbuf_addstr(sb, color_commit);
> + strbuf_addstr(sb, prefix);
>   while (decoration) {
> - strbuf_addstr(sb, color_commit);
> - strbuf_addstr(sb, prefix);
>   strbuf_addstr(sb, decorate_get_color(use_color, 
> decoration->type));
>   if (decoration->type == DECORATION_REF_TAG)
>   strbuf_addstr(sb, "tag: ");
>   strbuf_addstr(sb, decoration->name);
>   strbuf_addstr(sb, color_reset);
> - prefix = ", ";
> + strbuf_addstr(sb, color_commit);
> + if (decoration->next)
> + strbuf_addstr(sb, separator);
>   decoration = decoration->next;
>   }

I was kind of found of the nice trick to use a punctuation, which
first points at the prefix " (" and then later points at the
separator ", ", to allow the code that prefixes the punctuation
before showing a new item.  It is now lost.

We can restore it by doing something like this, though:

if (!decoration)
return;
while (decoration) {
strbuf_addstr(sb, prefix);
strbuf_addstr(sb, decoration->name);
prefix = separator;
decoration = decoration->next;
}

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index de0cc4a..38148c1 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -457,4 +457,15 @@ EOF
>   test_cmp expected actual1
>  '
>  
> +test_expect_success 'clean log decoration' '
> + git log --no-walk --tags --pretty="%H %D" --decorate=full >actual &&
> + cat  +$head1 tag: refs/tags/tag2
> +$head2 tag: refs/tags/message-one
> +$old_head1 tag: refs/tags/message-two
> +EOF

You could indent the here-doc if you wanted to, like this:

cat >expected <<-EOF &&
$head1 tag: ...
...
EOF

and the end result may look easier on the eyes.

> + sort actual >actual1 &&

Hmph.  I actually think the part that prepares the history makes
sure that the output order of the commits is predictable by using
test_commit and test_tick.  I see existing tests at the end (which
is a sign that they were added more recently than the rest of the
test script, and can indicate a careless addition) already has
"sort", but we shouldn't have to sort.

> + test_cmp expected actual1
> +'

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] Documentation/git-rebase.txt: make it explicit in the syntax there is no way to specify without .

[Junio C Hamano]

Sergey Organov  writes:

> Current syntax description makes one wonder if there is any syntactic way
> to distinguish between  and  so that one can specify
>  but not . The change makes it explicit that these
> arguments are in fact positional.

Makes sense.
Thanks.


>
> Signed-off-by: Sergey Organov 
> ---
>  Documentation/git-rebase.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f14100a..4138554 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  
>  [verse]
>  'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
> - [] []
> + [ []]
>  'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
>   --root []
>  'git rebase' --continue | --skip | --abort | --edit-todo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] refs: make rev-parse --quiet actually quiet

[Junio C Hamano]

David Aguilar  writes:

> This patch now has the t1503 test case squashed into it.
> It was previously a separate patch, but it makes more sense
> for them to go together.

Yes.

> diff --git a/refs.c b/refs.c
> index 2ce5d69..447e339 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3108,7 +3108,7 @@ static int read_ref_at_ent_oldest(unsigned char *osha1, 
> unsigned char *nsha1,
>   return 1;
>  }
>  
> -int read_ref_at(const char *refname, unsigned long at_time, int cnt,
> +int read_ref_at(const char *refname, unsigned int flags, unsigned long 
> at_time, int cnt,
>   unsigned char *sha1, char **msg,
>   unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt)
>  {
> @@ -3126,8 +3126,12 @@ int read_ref_at(const char *refname, unsigned long 
> at_time, int cnt,
>  
>   for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
>  
> - if (!cb.reccnt)
> - die("Log for %s is empty.", refname);
> + if (!cb.reccnt) {
> + if (flags & GET_SHA1_QUIETLY)
> + exit(1);

Do we want 1 or 128 just like die()?

> + else
> + die("Log for %s is empty.", refname);
> + }

Given that I see this behaviour:

$ git rev-parse da/rev-parse-verify-quiet@{1}
2892dfeec3f98f7e65a2746d271471d2c3c4af57
$ git rev-parse da/rev-parse-verify-quiet@{10}
fatal: Log for 'da/rev-parse-verify-quiet' only has 4 entries

and I do not see a change in this patch to touch "only has %d
entries" (found in sha1_name.c), I have to suspect that this change
is not sufficient to say "rev-parse --quiet will now be quiet".

$ git rev-parse --quiet --verify da/rev-parse-verify-quiet@{10.years.ago}

may want to return the oldest-known value as it has always done
without giving a warning.

There probably are other cases that relate to reflogs, e.g.

$ git rev-parse --quiet --verify @{-9}

to ask the tip of the branch you were on 9 "git checkout"s ago.
I think the last one does not have to be in the same commit as this
fix for "empty" and "you do not have that many" cases, though.

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 v5 00/23] Signed push

[Jaime Soriano Pastor]

On Tue, Sep 16, 2014 at 12:24 AM, Junio C Hamano  wrote:
> A failing test has been added at the end for smart HTTP.  It appears
> that somewhere in the callchain "--signed" is forgotten and the
> sending end not to send the certificate for some reason.  If
> somebody with a fresh set of eyes can look into it, that would be
> very much appreciated, as I do not expect I would have sufficient
> concentration to dig it quickly for several days at least.
>

It seems that smart HTTP is using "git send-pack" (called from
push_git()), that doesn't still know anything about signed pushes. I
think this is the point where the flag is forgotten.
--
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] credential-cache: close stderr in daemon process

[Junio C Hamano]

Jeff King  writes:

> Squash this in?

Yeah, I did a crude one without _errno() while sending the report;
will replace.

Thanks.

>
> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
> index c07a67c..c2f0049 100644
> --- a/credential-cache--daemon.c
> +++ b/credential-cache--daemon.c
> @@ -212,8 +212,10 @@ static void serve_cache(const char *socket_path, int 
> debug)
>  
>   printf("ok\n");
>   fclose(stdout);
> - if (!debug)
> - freopen("/dev/null", "w", stderr);
> + if (!debug) {
> + if (!freopen("/dev/null", "w", stderr))
> + die_errno("unable to point stderr to /dev/null");
> + }
>  
>   while (serve_cache_loop(fd))
>   ; /* nothing */
--
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: Apparent bug in git-gc

[Dale R. Worley]

> From: Jeff King 
> 
> > I have an 11 GB repository.  It passes git-fsck (though with a number
> > of dangling objects).  But when I run git-gc on it, the file
> > refs/heads/master disappears.
> 
> That's the expected behavior. Gc runs "git pack-refs", which puts an
> entry into packed-refs and prunes the loose ref.

Arrrgh, damn!  I knew that but I forgot it.  Sorry about the trouble.

(At least I don't have to worry about fixing it now.)

Dale
--
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] mailinfo: do not treat ">From" lines as in-body headers

[Junio C Hamano]

Jeff King  writes:

> The only cases that I can think of that would be a problem with this
> strictness are:
>
>   1. Somebody writes format-patch output to a file, reads in the mbox
>  using another program, and then writes out the result (munging the
>  mbox From line). And then pastes the whole thing into their email
>  body.
>
>  I can see the first part happening. But given that it is totally
>  irrelevant _unless_ they then screw up and paste the From line in
>  the body (which is already a corner case), it probably doesn't
>  matter.

Yeah, I tend to agree.

>   2. We change the static From lines that git generates. We can always
>  update the parser, of course, but it may be running a different
>  version of git than the sender.  People with an old git running
>  "git am" would stop skipping past "From" lines in messages from
>  people on newer gits.

I hope that it is not going to happen; the reason we refrain from
ever changing the datestamp has been to keep it constant to help
those who write magic(5), and I do not think we have a reason to
defeat that.

> I think you forgot to "git add" mbox.h. That being said, if we did go
> this route, I do not see any reason to share the code at all. This can
> be purely a mailinfo.c thing.

OK.  A reroll coming today when I find time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] pretty: add %D format specifier

[Harry Jeffery]

Add a new format specifier, '%D' that is identical in behaviour to '%d',
except that it does not include the ' (' prefix or ')' suffix provided
by '%d'.

Signed-off-by: Harry Jeffery 
---
 Documentation/pretty-formats.txt |  6 --
 log-tree.c   | 24 +---
 log-tree.h   |  8 +++-
 pretty.c |  4 
 t/t4205-log-pretty-formats.sh| 11 +++
 5 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index eac7909..2632e1a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -128,6 +128,7 @@ The placeholders are:
 - '%ct': committer date, UNIX timestamp
 - '%ci': committer date, ISO 8601 format
 - '%d': ref names, like the --decorate option of linkgit:git-log[1]
+- '%D': ref names without the " (", ")" wrapping.
 - '%e': encoding
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
@@ -182,8 +183,9 @@ The placeholders are:
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
 insert an empty string unless we are traversing reflog entries (e.g., by
-`git log -g`). The `%d` placeholder will use the "short" decoration
-format if `--decorate` was not already provided on the command line.
+`git log -g`). The `%d` and `%D` placeholders will use the "short"
+decoration format if `--decorate` was not already provided on the command
+line.
 
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
diff --git a/log-tree.c b/log-tree.c
index 95e9b1d..61d1dea 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -179,14 +179,16 @@ static void show_children(struct rev_info *opt, struct 
commit *commit, int abbre
 }
 
 /*
- * The caller makes sure there is no funny color before
- * calling. format_decorations makes sure the same after return.
+ * The caller makes sure there is no funny color before calling.
+ * format_decorations_extended makes sure the same after return.
  */
-void format_decorations(struct strbuf *sb,
+void format_decorations_extended(struct strbuf *sb,
const struct commit *commit,
-   int use_color)
+   int use_color,
+   const char *prefix,
+   const char *separator,
+   const char *suffix)
 {
-   const char *prefix;
struct name_decoration *decoration;
const char *color_commit =
diff_get_color(use_color, DIFF_COMMIT);
@@ -196,20 +198,20 @@ void format_decorations(struct strbuf *sb,
decoration = lookup_decoration(&name_decoration, &commit->object);
if (!decoration)
return;
-   prefix = " (";
+   strbuf_addstr(sb, color_commit);
+   strbuf_addstr(sb, prefix);
while (decoration) {
-   strbuf_addstr(sb, color_commit);
-   strbuf_addstr(sb, prefix);
strbuf_addstr(sb, decorate_get_color(use_color, 
decoration->type));
if (decoration->type == DECORATION_REF_TAG)
strbuf_addstr(sb, "tag: ");
strbuf_addstr(sb, decoration->name);
strbuf_addstr(sb, color_reset);
-   prefix = ", ";
+   strbuf_addstr(sb, color_commit);
+   if (decoration->next)
+   strbuf_addstr(sb, separator);
decoration = decoration->next;
}
-   strbuf_addstr(sb, color_commit);
-   strbuf_addch(sb, ')');
+   strbuf_addstr(sb, suffix);
strbuf_addstr(sb, color_reset);
 }
 
diff --git a/log-tree.h b/log-tree.h
index d6ecd4d..b26160c 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,7 +13,13 @@ int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
 void show_log(struct rev_info *opt);
-void format_decorations(struct strbuf *sb, const struct commit *commit, int 
use_color);
+void format_decorations_extended(struct strbuf *sb, const struct commit 
*commit,
+int use_color,
+const char *prefix,
+const char *separator,
+const char *suffix);
+#define format_decorations(strbuf, commit, color) \
+format_decorations_extended((strbuf), (commit), 
(color), " (", ", ", ")")
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 const char **subject_p,
diff --git a/pretty.c b/pretty.c
index 44b9f64..46d65b9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1197,6 +1197,10 @@ static size_t format_commit_one(struct strbuf *

[PATCH] Documentation/git-rebase.txt: make it explicit in the syntax there is no way to specify without .

[Sergey Organov]

Current syntax description makes one wonder if there is any syntactic way
to distinguish between  and  so that one can specify
 but not . The change makes it explicit that these
arguments are in fact positional.

Signed-off-by: Sergey Organov 
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f14100a..4138554 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 
 [verse]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
-   [] []
+   [ []]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
--root []
 'git rebase' --continue | --skip | --abort | --edit-todo
-- 
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: Administrator Warning

[Luis A. Beltran]

From: Luis A. Beltran
Sent: Tuesday, September 16, 2014 6:39 AM
To: Luis A. Beltran
Subject: Administrator Warning

Help desk will undergo unscheduled system maintenance today in order to improve 
your account. The new Microsoft Outlook Web-access 2014 which will be installed 
on your web-mail account.

Your present account will be deactivated to create space for the new web-access 
2014. In other to complete this process, please 
CLICKHERE and complete the survey.

Your account will be inactive if this survey is not completed.


Thank you.
Web-mail Administrator
--
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 v14 01/11] trailer: add data structures and basic functions

[Christian Couder]

On Mon, Sep 15, 2014 at 10:39 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> +/* Get the length of buf from its beginning until its last alphanumeric 
>> character */
>
> That makes it sound as if feeding "abc%de#f@" to the function returns
> 3 for "abc", but

For me the last alphanumeric character in "abc%de#f@" is "f", so it is
the length from the beginning to "f" so it should return 8.

>> +static size_t alnum_len(const char *buf, size_t len)
>> +{
>> + while (len > 0 && !isalnum(buf[len - 1]))
>> + len--;
>> + return len;
>> +}
>
> doesn't it look at '@', be unhappy and decrement, look at 'f' and
> break out to return the length of "abc%de#f"?

Yeah, that's the expected behavior.

> Perhaps that behaviour _is_ what you want, but then the comment is
> lying, no?

I don't think so, but maybe you are parsing the comment in a different
way than I am.
What would you suggest instead?

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