* Re: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Martin Fick @ 2013-01-03 23:52 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Shawn Pearce, Jeff King, git, Junio C Hamano
In-Reply-To: <201212310330.53835.mfick@codeaurora.org>
Any thoughts on this idea? Is it flawed? I am trying to
write it up in a more formal generalized manner and was
hoping to get at least one "it seems sane" before I do.
Thanks,
-Martin
On Monday, December 31, 2012 03:30:53 am Martin Fick wrote:
> On Thursday, December 27, 2012 04:11:51 pm Martin Fick
wrote:
> > It concerns me that git uses any locking at all, even
> > for refs since it has the potential to leave around
> > stale locks.
> > ...
> > [a previous not so great attempt to fix this]
> > ...
>
> I may have finally figured out a working loose ref update
> mechanism which I think can avoid stale locks.
> Unfortunately it requires atomic directory renames and
> universally unique identifiers (uuids). These may be
> no-go criteria? But I figure it is worth at least
> exploring this idea because of the potential benefits?
>
> The general approach is to setup a transaction and either
> commit or abort it. A transaction can be setup by
> renaming an appropriately setup directory to the
> "ref.lock" name. If the rename succeeds, the transaction
> is begun. Any actor can abort the transaction (up until
> it is committed) by simply deleting the "ref.lock"
> directory, so it is not at risk of going stale. However,
> once the actor who sets up the transaction commits it,
> deleting the "ref.lock" directory simply aids in cleaning
> it up for the next transaction (instead of aborting it).
>
> One important piece of the transaction is the use of
> uuids. The uuids provide a mechanism to tie the atomic
> commit pieces to the transactions and thus to prevent
> long sleeping process from inadvertently performing
> actions which could be out of date when they wake finally
> up. In each case, the atomic commit piece is the
> renaming of a file. For the create and update pieces, a
> file is renamed from the "ref.lock" dir to the "ref" file
> resulting in an update to the sha for the ref. However,
> in the delete case, the "ref" file is instead renamed to
> end up in the "ref.lock" directory resulting in a delete
> of the ref. This scheme does not affect the way refs are
> read today,
>
> To prepare for a transaction, an actor first generates a
> uuid (an exercise I will delay for now). Next, a tmp
> directory named after the uuid is generated in the parent
> directory for the ref to be updated, perhaps something
> like: ".lock_uuid". In this directory is places either a
> file or a directory named after the uuid, something like:
> ".lock_uuid/,uuid". In the case of a create or an
> update, the new sha is written to this file. In the case
> of a delete, it is a directory.
>
> Once the tmp directory is setup, the initiating actor
> attempts to start the transaction by renaming the tmp
> directory to "ref.lock". If the rename fails, the update
> fails. If the rename succeeds, the actor can then attempt
> to commit the transaction (before another actor aborts
> it).
>
> In the case of a create, the actor verifies that "ref"
> does not currently exist, and then renames the now named
> "ref.lock/uuid" file to "ref". On success, the ref was
> created.
>
> In the case of an update, the actor verifies that "ref"
> currently contains the old sha, and then also renames the
> now named "ref.lock/uuid" file to "ref". On success, the
> ref was updated.
>
> In the case of a delete, the actor may verify that "ref"
> currently contains the sha to "prune" if it needs to, and
> then renames the "ref" file to "ref.lock/uuid/delete". On
> success, the ref was deleted.
>
> Whether successful or not, the actor may now simply delete
> the "ref.lock" directory, clearing the way for a new
> transaction. Any other actor may delete this directory at
> any time also, likely either on conflict (if they are
> attempting to initiate a transaction), or after a grace
> period just to cleanup the FS. Any actor may also safely
> cleanup the tmp directories, preferably also after a grace
> period.
>
> One neat part about this scheme is that I believe it would
> be backwards compatible with the current locking
> mechanism since the transaction directory will simply
> appear to be a lock to older clients. And the old lock
> file should continue to lock out these newer
> transactions.
>
> Due to this backwards compatibility, I believe that this
> could be incrementally employed today without affecting
> very much. It could be deployed in place of any updates
> which only hold ref.locks to update the loose ref. So
> for example I think it could replace step 4a below from
> Michael Haggerty's description of today's loose ref
> pruning during
>
> ref packing:
> > * Pack references:
> ...
>
> > 4. prune_refs(): for each ref in the ref_to_prune list,
> >
> > call prune_ref():
> > a. Lock the reference using lock_ref_sha1(),
> > verifying that the recorded SHA1 is still valid. If
> > it is, unlink the loose reference file then free
> > the lock; otherwise leave the loose reference file
> > untouched.
>
> I think it would also therefore be able to replace the
> loose ref locking in Michael's new ref-packing scheme as
> well as the locking in Michael's new ref deletion scheme
> (again steps
>
> 4):
> > * Delete reference foo:
> ...
>
> > 4. Delete loose ref for "foo":
> > a. Acquire the lock $GIT_DIR/refs/heads/foo.lock
> >
> > b. Unlink $GIT_DIR/refs/heads/foo if it is
> > unchanged.
> >
> > If it is changed, leave it untouched. If it is
> > deleted,
> >
> > that is OK too.
> >
> > c. Release lock $GIT_DIR/refs/heads/foo.lock
>
> ...
>
> > * Pack references:
> ...
>
> > 4. prune_refs(): for each ref in the ref_to_prune
> > list,
> >
> > call prune_ref():
> > a. Lock the loose reference using lock_ref_sha1(),
> >
> > verifying that the recorded SHA1 is still valid
> >
> > b. If it is, unlink the loose reference file
> >
> > (otherwise, leave it untouched)
> >
> > c. Release the lock on the loose reference
>
> To be honest, I suspect I missed something obvious because
> this seems almost too simple to work. I am ashamed that
> it took me so long to come up with (of course, I will be
> even more ashamed :( when it is shown to be flawed!)
> This scheme also feels extensible. if there are no
> obvious flaws in it, I will try to post solutions for ref
> packing and for multiple repository/ref transactions also
> soon.
>
> I welcome any comments/criticisms,
>
> -Martin
> --
> To unsubscribe from this list: send the line "unsubscribe
> git" in the body of a message to
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Pretty pictures of git merge conflicts
From: John Szakmeister @ 2013-01-03 23:53 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <50E5B3BE.7080500@alum.mit.edu>
On Thu, Jan 3, 2013 at 11:37 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Hi,
>
> I've been thinking lately about how to attack difficult git merge
> conflicts. The first step is to visualize them. I have written some
> articles [1,2,3] describing a way to atomize a complicated merge and
> efficiently compute diagrams that show which pairwise commits cause the
> merge to go awry. I hope you find them interesting; feedback would be
> very welcome.
>
> I am working on some more articles (including software) that I plan to
> grow into tools to help users perform git merges that would otherwise
> seem hopeless.
Interesting work Michael! Interestingly, I've taken such an approach
in a few real life situations. It was a real help it trying to get
the conflicts resolved, so I'm eager to see the tools you come up
with!
-John
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: give --reroll-count a short synonym -v
From: Junio C Hamano @ 2013-01-04 0:00 UTC (permalink / raw)
To: Philip Oakley; +Cc: git
In-Reply-To: <A6A2DEC8F88743B88AA03DC5BD8547B7@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
>> +test_expect_success 'reroll count (-v)' '
>> + rm -fr patches &&
>> + git format-patch -o patches --cover-letter -v 4 master..side >list
>> &&
>
> Shouldn't this be using the sticked form -v4 as described in the
> commit message and gitcli?
I personally do not care too deeply either way.
Both styles seem to work, and if/when we break parse-options API
implementation, this test will break and we will notice, which might
be an added plus ;-)
^ permalink raw reply
* Fwd: Git hangs after resolving deltas when using NTLM proxy on Windows
From: Adam Baxter @ 2013-01-04 0:28 UTC (permalink / raw)
To: git
In-Reply-To: <CAChFQ9QYt6ncpD2zwQu+ziD98J+aOd4TWAJR9dLdDs69bskZ2g@mail.gmail.com>
Hi,
Git is authenticating to my corporate proxy correctly, but is hanging
after "resolving deltas".
This is when cloning via HTTPS.
Apologies for linking to a Gist, but I'm having difficulty getting
this list to accept a log file attachment.
See https://gist.github.com/4448684 for the curl log.
Regards,
Adam
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: give --reroll-count a short synonym -v
From: Philip Oakley @ 2013-01-03 23:21 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <1357166525-12188-2-git-send-email-gitster@pobox.com>
From: "Junio C Hamano" <gitster@pobox.com> Sent: Wednesday, January 02,
2013 10:42 PM
> Accept "-v" as a synonym to "--reroll-count", so that users can say
> "git format-patch -v4 master", instead of having to fully spell it
> out as "git format-patch --reroll-count=4 master".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> * As I do not think of a reason why users would want to tell the
> command to be "verbose", I think it may be OK to squat on the
> short and sweet single letter option, but I do not mind dropping
> it.
>
> Documentation/git-format-patch.txt | 3 ++-
> builtin/log.c | 2 +-
> t/t4014-format-patch.sh | 8 ++++++++
> 3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index 736d8bf..ae3212e 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -18,7 +18,7 @@ SYNOPSIS
> [--start-number <n>] [--numbered-files]
> [--in-reply-to=Message-Id] [--suffix=.<sfx>]
> [--ignore-if-in-upstream]
> - [--subject-prefix=Subject-Prefix] [--reroll-count <n>]
> + [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]
> [--to=<email>] [--cc=<email>]
> [--cover-letter] [--quiet]
> [<common diff options>]
> @@ -166,6 +166,7 @@ will want to ensure that threading is disabled for
> `git send-email`.
> allows for useful naming of a patch series, and can be
> combined with the `--numbered` option.
>
> +-v <n>::
> --reroll-count=<n>::
> Mark the series as the <n>-th iteration of the topic. The
> output filenames have `v<n>` pretended to them, and the
> diff --git a/builtin/log.c b/builtin/log.c
> index e101498..08e8a9d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1081,7 +1081,7 @@ int cmd_format_patch(int argc, const char
> **argv, const char *prefix)
> N_("use <sfx> instead of '.patch'")),
> OPT_INTEGER(0, "start-number", &start_number,
> N_("start numbering patches at <n> instead of 1")),
> - OPT_INTEGER(0, "reroll-count", &reroll_count,
> + OPT_INTEGER('v', "reroll-count", &reroll_count,
> N_("mark the series as Nth re-roll")),
> { OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
> N_("Use [<prefix>] instead of [PATCH]"),
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 0ff9958..03b8e51 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -245,6 +245,14 @@ test_expect_success 'reroll count' '
> ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> '
>
> +test_expect_success 'reroll count (-v)' '
> + rm -fr patches &&
> + git format-patch -o patches --cover-letter -v 4 master..side >list
> &&
Shouldn't this be using the sticked form -v4 as described in the commit
message and gitcli?
Or is this being too picky?
> + ! grep -v "^patches/v4-000[0-3]-" list &&
> + sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> + ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> +'
> +
> check_threading () {
> expect="$1" &&
> shift &&
> --
> 1.8.0.9.g5e84801
>
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: give --reroll-count a short synonym -v
From: Junio C Hamano @ 2013-01-04 1:28 UTC (permalink / raw)
To: Philip Oakley; +Cc: git
In-Reply-To: <7vpq1lvmtz.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>> +test_expect_success 'reroll count (-v)' '
>>> + rm -fr patches &&
>>> + git format-patch -o patches --cover-letter -v 4 master..side >list
>>> &&
>>
>> Shouldn't this be using the sticked form -v4 as described in the
>> commit message and gitcli?
>
> I personally do not care too deeply either way.
Actually, I do care. And in this case both should work.
Separating argument from the option
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
You can write the mandatory option parameter to an option as a separate
word on the command line. That means that all the following uses work:
----------------------------
$ git foo --long-opt=Arg
$ git foo --long-opt Arg
$ git foo -oArg
$ git foo -o Arg
----------------------------
As "reroll-count" must always be followed by nth (in other words, it
is not optional), the following does not apply.
However, this is *NOT* allowed for switches with an optional value, where the
'sticked' form must be used:
^ permalink raw reply
* [RFC/PATCH] avoid SIGPIPE warnings for aliases
From: Jeff King @ 2013-01-04 12:47 UTC (permalink / raw)
To: git; +Cc: Bart Trojanowski
When git executes an alias that specifies an external
command, it will complain if the alias dies due to a signal.
This is usually a good thing, as signal deaths are
unexpected. However, SIGPIPE is not unexpected for many
commands which produce a lot of output; it is intended that
the user closing the pager would kill them them via SIGPIPE.
As a result, the user might see annoying messages in a
scenario like this:
$ cat ~/.gitconfig
[alias]
lgbase = log --some-options
lg = !git lgbase --more-options
lg2 = !git lgbase --other-options
$ git lg -p
[user hits 'q' to exit pager]
error: git lgbase --more-options died of signal 13
fatal: While expanding alias 'lg': 'git lgbase --more-options': Success
Many users won't see this, because we execute the external
command with the shell, and a POSIX shell will silently
rewrite the signal-death exit code into 128+signal, and we
will treat it like a normal exit code. However, this does
not always happen:
1. If the sub-command does not have shell metacharacters,
we will skip the shell and exec it directly, getting
the signal code.
2. Some shells do not do this rewriting; for example,
setting SHELL_PATH set to zsh will reveal this problem.
This patch squelches the message, and let's git exit
silently (with the same exit code that a shell would use in
case of a signal).
The first line of the message comes from run-command's
wait_or_whine. To silence that message, we remain quiet
anytime we see SIGPIPE.
The second line comes from handle_alias itself. It calls
die_errno whenever run_command returns a negative value.
However, only -1 indicates a syscall error where errno has
something useful (note that it says the useless "success"
above). Instead, we treat negative returns from run_command
(except for -1) as a normal code to be passed to exit.
Signed-off-by: Jeff King <peff@peff.net>
---
I have two reservations with this patch:
1. We are ignoring SIGPIPE all the time. For an alias that is calling
"log", that is fine. But if pack-objects dies on the server side,
seeing that it died from SIGPIPE is useful data, and we are
squelching that. Maybe callers of run-command should have to pass
an "ignore SIGPIPE" flag?
2. The die_errno in handle_alias is definitely wrong. Even if we want
to print a message for signal death, showing errno is bogus unless
the return value was -1. But is it the right thing to just pass the
negative value straight to exit()? It works, but it is depending on
the fact that (unsigned char)(ret & 0xff) behaves in a certain way
(i.e., that we are on a twos-complement platform, and -13 becomes
141). That is not strictly portable, but it is probably fine in
practice. I'd worry more about exit() doing something weird on
Windows.
git.c | 2 +-
run-command.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index d33f9b3..07edb8a 100644
--- a/git.c
+++ b/git.c
@@ -175,7 +175,7 @@ static int handle_alias(int *argcp, const char ***argv)
alias_argv[argc] = NULL;
ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
- if (ret >= 0) /* normal exit */
+ if (ret != -1) /* normal exit */
exit(ret);
die_errno("While expanding alias '%s': '%s'",
diff --git a/run-command.c b/run-command.c
index 757f263..01a4c9b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -242,7 +242,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
error("waitpid is confused (%s)", argv0);
} else if (WIFSIGNALED(status)) {
code = WTERMSIG(status);
- if (code != SIGINT && code != SIGQUIT)
+ if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
error("%s died of signal %d", argv0, code);
/*
* This return value is chosen so that code & 0xff
--
1.8.1.rc1.16.g6d46841
^ permalink raw reply related
* [PATCH] attr: allow pattern escape using backslashes
From: Nguyễn Thái Ngọc Duy @ 2013-01-04 14:46 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Patterns in .gitattributes are separated by whitespaces, which makes
it impossible to specify exact spaces in the pattern. '?' can be used
as a workaround, but it matches other characters too. This patch makes
a space following a backslash part of the pattern, not a pattern
separator.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
A similar patch was posted twice (during freeze time iirc). I think
this is a good change, so I will keep reposting until someone turns
it down.
We could use wildmatch for parsing here, which would support patterns
like "Hello[ ]world". But that's not going to happen until wildmatch
graduates and somebody brings it up again.
Documentation/gitattributes.txt | 6 +++---
attr.c | 8 +++++++-
t/t0003-attributes.sh | 5 +++++
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2698f63..113b1f8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -20,9 +20,9 @@ Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
-That is, a pattern followed by an attributes list,
-separated by whitespaces. When the pattern matches the
-path in question, the attributes listed on the line are given to
+That is, a pattern followed by an attributes list, separated by
+whitespaces that are not quoted by a backslash. When the pattern matches
+the path in question, the attributes listed on the line are given to
the path.
Each attribute can be in one of these states for a given path:
diff --git a/attr.c b/attr.c
index 097ae87..776f34e 100644
--- a/attr.c
+++ b/attr.c
@@ -209,7 +209,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (!*cp || *cp == '#')
return NULL;
name = cp;
- namelen = strcspn(name, blank);
+ namelen = 0;
+ while (name[namelen] != '\0' && !strchr(blank, name[namelen])) {
+ if (name[namelen] == '\\' && name[namelen + 1] != '\0')
+ namelen += 2;
+ else
+ namelen++;
+ }
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
!prefixcmp(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 807b8b8..6a5d8ab 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -24,6 +24,7 @@ test_expect_success 'setup' '
echo "offon -test test"
echo "no notest"
echo "A/e/F test=A/e/F"
+ echo "A\\ b test=space"
) >.gitattributes &&
(
echo "g test=a/g" &&
@@ -196,6 +197,10 @@ test_expect_success 'root subdir attribute test' '
attr_check subdir/a/i unspecified
'
+test_expect_success 'quoting in pattern' '
+ attr_check "A b" space
+'
+
test_expect_success 'negative patterns' '
echo "!f test=bar" >.gitattributes &&
test_must_fail git check-attr test -- f
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases
From: Johannes Sixt @ 2013-01-04 16:55 UTC (permalink / raw)
To: Jeff King; +Cc: git, Bart Trojanowski
In-Reply-To: <20130104124756.GA402@sigill.intra.peff.net>
Am 04.01.2013 13:47, schrieb Jeff King:
> I have two reservations with this patch:
>
> 1. We are ignoring SIGPIPE all the time. For an alias that is calling
> "log", that is fine. But if pack-objects dies on the server side,
> seeing that it died from SIGPIPE is useful data, and we are
> squelching that. Maybe callers of run-command should have to pass
> an "ignore SIGPIPE" flag?
I am of two minds. On the one hand, losing useful debugging information
is not something we should do lightly. On the other hand, the message is
really noise most of the time, even on servers: when pack-objects dies
on the server side, it is most likely due to a connection that breaks
(voluntarily or involuntarily) half-way during a transfer, and is
presumably a frequent event, and as such not worth noting most of the time.
> 2. The die_errno in handle_alias is definitely wrong. Even if we want
> to print a message for signal death, showing errno is bogus unless
> the return value was -1. But is it the right thing to just pass the
> negative value straight to exit()? It works, but it is depending on
> the fact that (unsigned char)(ret & 0xff) behaves in a certain way
> (i.e., that we are on a twos-complement platform, and -13 becomes
> 141). That is not strictly portable, but it is probably fine in
> practice. I'd worry more about exit() doing something weird on
> Windows.
It did something weird on Windows until we added this line to
compat/mingw.h:
#define exit(code) exit((code) & 0xff)
-- Hannes
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2013, #02; Thu, 3)
From: Adam Spiers @ 2013-01-04 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmwwqvzy4.fsf@alter.siamese.dyndns.org>
On Thu, Jan 3, 2013 at 7:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * as/check-ignore (2012-12-28) 19 commits
> - Add git-check-ignore sub-command
> - setup.c: document get_pathspec()
> - pathspec.c: extract new validate_path() for reuse
> - pathspec.c: move reusable code from builtin/add.c
> - add.c: remove unused argument from validate_pathspec()
> - add.c: refactor treat_gitlinks()
> - dir.c: provide clear_directory() for reclaiming dir_struct memory
> - dir.c: keep track of where patterns came from
> - dir.c: use a single struct exclude_list per source of excludes
> - dir.c: rename free_excludes() to clear_exclude_list()
> - dir.c: refactor is_path_excluded()
> - dir.c: refactor is_excluded()
> - dir.c: refactor is_excluded_from_list()
> - dir.c: rename excluded() to is_excluded()
> - dir.c: rename excluded_from_list() to is_excluded_from_list()
> - dir.c: rename path_excluded() to is_path_excluded()
> - dir.c: rename cryptic 'which' variable to more consistent name
> - Improve documentation and comments regarding directory traversal API
> - api-directory-listing.txt: update to match code
>
> Rerolled. The early parts looked mostly fine; we may want to split
> this into two topics and have the early half graduate sooner.
Sounds good to me. As already mentioned, I have the v4 series ready
and it addresses all issues already voiced in v3, but I have postponed
submitting it as per your request. Please let me know when and how to
proceed, thanks!
^ permalink raw reply
* RE: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Pyeron, Jason J CTR (US) @ 2013-01-04 17:52 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <201301031652.44982.mfick@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 7382 bytes --]
> From: Martin Fick
> Sent: Thursday, January 03, 2013 6:53 PM
>
> Any thoughts on this idea? Is it flawed? I am trying to
> write it up in a more formal generalized manner and was
> hoping to get at least one "it seems sane" before I do.
If you are assuming that atomic renames, etc. are available, then you should identify a test case and a degrade operation path when it is not available.
>
> Thanks,
>
> -Martin
>
> On Monday, December 31, 2012 03:30:53 am Martin Fick wrote:
> > On Thursday, December 27, 2012 04:11:51 pm Martin Fick
> wrote:
> > > It concerns me that git uses any locking at all, even
> > > for refs since it has the potential to leave around
> > > stale locks.
> > > ...
> > > [a previous not so great attempt to fix this]
> > > ...
> >
> > I may have finally figured out a working loose ref update
> > mechanism which I think can avoid stale locks.
> > Unfortunately it requires atomic directory renames and
> > universally unique identifiers (uuids). These may be
> > no-go criteria? But I figure it is worth at least
> > exploring this idea because of the potential benefits?
> >
> > The general approach is to setup a transaction and either
> > commit or abort it. A transaction can be setup by
> > renaming an appropriately setup directory to the
> > "ref.lock" name. If the rename succeeds, the transaction
> > is begun. Any actor can abort the transaction (up until
> > it is committed) by simply deleting the "ref.lock"
> > directory, so it is not at risk of going stale. However,
> > once the actor who sets up the transaction commits it,
> > deleting the "ref.lock" directory simply aids in cleaning
> > it up for the next transaction (instead of aborting it).
> >
> > One important piece of the transaction is the use of
> > uuids. The uuids provide a mechanism to tie the atomic
> > commit pieces to the transactions and thus to prevent
> > long sleeping process from inadvertently performing
> > actions which could be out of date when they wake finally
> > up. In each case, the atomic commit piece is the
> > renaming of a file. For the create and update pieces, a
> > file is renamed from the "ref.lock" dir to the "ref" file
> > resulting in an update to the sha for the ref. However,
> > in the delete case, the "ref" file is instead renamed to
> > end up in the "ref.lock" directory resulting in a delete
> > of the ref. This scheme does not affect the way refs are
> > read today,
> >
> > To prepare for a transaction, an actor first generates a
> > uuid (an exercise I will delay for now). Next, a tmp
> > directory named after the uuid is generated in the parent
> > directory for the ref to be updated, perhaps something
> > like: ".lock_uuid". In this directory is places either a
> > file or a directory named after the uuid, something like:
> > ".lock_uuid/,uuid". In the case of a create or an
> > update, the new sha is written to this file. In the case
> > of a delete, it is a directory.
> >
> > Once the tmp directory is setup, the initiating actor
> > attempts to start the transaction by renaming the tmp
> > directory to "ref.lock". If the rename fails, the update
> > fails. If the rename succeeds, the actor can then attempt
> > to commit the transaction (before another actor aborts
> > it).
> >
> > In the case of a create, the actor verifies that "ref"
> > does not currently exist, and then renames the now named
> > "ref.lock/uuid" file to "ref". On success, the ref was
> > created.
> >
> > In the case of an update, the actor verifies that "ref"
> > currently contains the old sha, and then also renames the
> > now named "ref.lock/uuid" file to "ref". On success, the
> > ref was updated.
> >
> > In the case of a delete, the actor may verify that "ref"
> > currently contains the sha to "prune" if it needs to, and
> > then renames the "ref" file to "ref.lock/uuid/delete". On
> > success, the ref was deleted.
> >
> > Whether successful or not, the actor may now simply delete
> > the "ref.lock" directory, clearing the way for a new
> > transaction. Any other actor may delete this directory at
> > any time also, likely either on conflict (if they are
> > attempting to initiate a transaction), or after a grace
> > period just to cleanup the FS. Any actor may also safely
> > cleanup the tmp directories, preferably also after a grace
> > period.
> >
> > One neat part about this scheme is that I believe it would
> > be backwards compatible with the current locking
> > mechanism since the transaction directory will simply
> > appear to be a lock to older clients. And the old lock
> > file should continue to lock out these newer
> > transactions.
> >
> > Due to this backwards compatibility, I believe that this
> > could be incrementally employed today without affecting
> > very much. It could be deployed in place of any updates
> > which only hold ref.locks to update the loose ref. So
> > for example I think it could replace step 4a below from
> > Michael Haggerty's description of today's loose ref
> > pruning during
> >
> > ref packing:
> > > * Pack references:
> > ...
> >
> > > 4. prune_refs(): for each ref in the ref_to_prune list,
> > >
> > > call prune_ref():
> > > a. Lock the reference using lock_ref_sha1(),
> > > verifying that the recorded SHA1 is still valid. If
> > > it is, unlink the loose reference file then free
> > > the lock; otherwise leave the loose reference file
> > > untouched.
> >
> > I think it would also therefore be able to replace the
> > loose ref locking in Michael's new ref-packing scheme as
> > well as the locking in Michael's new ref deletion scheme
> > (again steps
> >
> > 4):
> > > * Delete reference foo:
> > ...
> >
> > > 4. Delete loose ref for "foo":
> > > a. Acquire the lock $GIT_DIR/refs/heads/foo.lock
> > >
> > > b. Unlink $GIT_DIR/refs/heads/foo if it is
> > > unchanged.
> > >
> > > If it is changed, leave it untouched. If it is
> > > deleted,
> > >
> > > that is OK too.
> > >
> > > c. Release lock $GIT_DIR/refs/heads/foo.lock
> >
> > ...
> >
> > > * Pack references:
> > ...
> >
> > > 4. prune_refs(): for each ref in the ref_to_prune
> > > list,
> > >
> > > call prune_ref():
> > > a. Lock the loose reference using lock_ref_sha1(),
> > >
> > > verifying that the recorded SHA1 is still valid
> > >
> > > b. If it is, unlink the loose reference file
> > >
> > > (otherwise, leave it untouched)
> > >
> > > c. Release the lock on the loose reference
> >
> > To be honest, I suspect I missed something obvious because
> > this seems almost too simple to work. I am ashamed that
> > it took me so long to come up with (of course, I will be
> > even more ashamed :( when it is shown to be flawed!)
> > This scheme also feels extensible. if there are no
> > obvious flaws in it, I will try to post solutions for ref
> > packing and for multiple repository/ref transactions also
> > soon.
> >
> > I welcome any comments/criticisms,
> >
> > -Martin
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > git" in the body of a message to
> > majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]
^ permalink raw reply
* Re: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Martin Fick @ 2013-01-04 18:01 UTC (permalink / raw)
To: Pyeron, Jason J CTR (US); +Cc: git@vger.kernel.org
In-Reply-To: <871B6C10EBEFE342A772D1159D1320853A011469@umechphj.easf.csd.disa.mil>
On Friday, January 04, 2013 10:52:43 am Pyeron, Jason J
CTR (US) wrote:
> > From: Martin Fick
> > Sent: Thursday, January 03, 2013 6:53 PM
> >
> > Any thoughts on this idea? Is it flawed? I am
trying
> > to write it up in a more formal generalized manner
and
> > was hoping to get at least one "it seems sane"
before
> > I do.
>
> If you are assuming that atomic renames, etc. are
> available, then you should identify a test case and a
> degrade operation path when it is not available.
Thanks, sound reasonable. Where you thinking a runtime
test case that would be run before every transaction? I
was anticipating a per repo config option called
something like "core.locks = recoverable" that would be
needed to turn them on? I was thinking that this was
something that server sites could test in advance on
their repos and then enable it for them. Maybe a git-
lock tool with a --test-recoverable option?
-Martin
> >
> > On Monday, December 31, 2012 03:30:53 am Martin Fick
wrote:
> > > On Thursday, December 27, 2012 04:11:51 pm Martin
> > > Fick
> >
> > wrote:
> > > > It concerns me that git uses any locking at all,
> > > > even for refs since it has the potential to
leave
> > > > around stale locks.
> > > > ...
> > > > [a previous not so great attempt to fix this]
> > > > ...
> > >
> > > I may have finally figured out a working loose ref
> > > update mechanism which I think can avoid stale
> > > locks. Unfortunately it requires atomic directory
> > > renames and universally unique identifiers
(uuids).
> > > These may be no-go criteria? But I figure it is
> > > worth at least exploring this idea because of the
> > > potential benefits?
> > >
> > > The general approach is to setup a transaction and
> > > either commit or abort it. A transaction can be
> > > setup by renaming an appropriately setup directory
> > > to the "ref.lock" name. If the rename succeeds,
the
> > > transaction is begun. Any actor can abort the
> > > transaction (up until it is committed) by simply
> > > deleting the "ref.lock" directory, so it is not at
> > > risk of going stale. However, once the actor who
> > > sets up the transaction commits it, deleting the
> > > "ref.lock" directory simply aids in cleaning it up
> > > for the next transaction (instead of aborting it).
> > >
> > > One important piece of the transaction is the use
of
> > > uuids. The uuids provide a mechanism to tie the
> > > atomic commit pieces to the transactions and thus
to
> > > prevent long sleeping process from inadvertently
> > > performing actions which could be out of date when
> > > they wake finally up. In each case, the atomic
> > > commit piece is the renaming of a file. For the
> > > create and update pieces, a file is renamed from
the
> > > "ref.lock" dir to the "ref" file resulting in an
> > > update to the sha for the ref. However, in the
> > > delete case, the "ref" file is instead renamed to
> > > end up in the "ref.lock" directory resulting in a
> > > delete of the ref. This scheme does not affect
the
> > > way refs are read today,
> > >
> > > To prepare for a transaction, an actor first
> > > generates a uuid (an exercise I will delay for
now).
> > > Next, a tmp directory named after the uuid is
> > > generated in the parent directory for the ref to
be
> > > updated, perhaps something like: ".lock_uuid". In
> > > this directory is places either a file or a
> > > directory named after the uuid, something like:
> > > ".lock_uuid/,uuid". In the case of a create or an
> > > update, the new sha is written to this file. In
the
> > > case of a delete, it is a directory.
> > >
> > > Once the tmp directory is setup, the initiating
actor
> > > attempts to start the transaction by renaming the
tmp
> > > directory to "ref.lock". If the rename fails, the
> > > update fails. If the rename succeeds, the actor
can
> > > then attempt to commit the transaction (before
> > > another actor aborts it).
> > >
> > > In the case of a create, the actor verifies that
> > > "ref" does not currently exist, and then renames
the
> > > now named "ref.lock/uuid" file to "ref". On
success,
> > > the ref was created.
> > >
> > > In the case of an update, the actor verifies that
> > > "ref" currently contains the old sha, and then
also
> > > renames the now named "ref.lock/uuid" file to
"ref".
> > > On success, the ref was updated.
> > >
> > > In the case of a delete, the actor may verify that
> > > "ref" currently contains the sha to "prune" if it
> > > needs to, and then renames the "ref" file to
> > > "ref.lock/uuid/delete". On success, the ref was
> > > deleted.
> > >
> > > Whether successful or not, the actor may now
simply
> > > delete the "ref.lock" directory, clearing the way
> > > for a new transaction. Any other actor may delete
> > > this directory at any time also, likely either on
> > > conflict (if they are attempting to initiate a
> > > transaction), or after a grace period just to
> > > cleanup the FS. Any actor may also safely cleanup
> > > the tmp directories, preferably also after a grace
> > > period.
> > >
> > > One neat part about this scheme is that I believe
it
> > > would be backwards compatible with the current
> > > locking mechanism since the transaction directory
> > > will simply appear to be a lock to older clients.
> > > And the old lock file should continue to lock out
> > > these newer transactions.
> > >
> > > Due to this backwards compatibility, I believe
that
> > > this could be incrementally employed today without
> > > affecting very much. It could be deployed in
place
> > > of any updates which only hold ref.locks to update
> > > the loose ref. So for example I think it could
> > > replace step 4a below from Michael Haggerty's
> > > description of today's loose ref pruning during
> > >
> > > ref packing:
> > > > * Pack references:
> > > ...
> > >
> > > > 4. prune_refs(): for each ref in the
ref_to_prune
> > > > list,
> > > >
> > > > call prune_ref():
> > > > a. Lock the reference using lock_ref_sha1(),
> > > > verifying that the recorded SHA1 is still
> > > > valid. If it is, unlink the loose reference
> > > > file then free the lock; otherwise leave the
> > > > loose reference file untouched.
> > >
> > > I think it would also therefore be able to replace
> > > the loose ref locking in Michael's new ref-packing
> > > scheme as well as the locking in Michael's new ref
> > > deletion scheme (again steps
> > >
> > > 4):
> > > > * Delete reference foo:
> > > ...
> > >
> > > > 4. Delete loose ref for "foo":
> > > > a. Acquire the lock
> > > > $GIT_DIR/refs/heads/foo.lock
> > > >
> > > > b. Unlink $GIT_DIR/refs/heads/foo if it is
> > > > unchanged.
> > > >
> > > > If it is changed, leave it untouched. If it is
> > > > deleted,
> > > >
> > > > that is OK too.
> > > >
> > > > c. Release lock
$GIT_DIR/refs/heads/foo.lock
> > >
> > > ...
> > >
> > > > * Pack references:
> > > ...
> > >
> > > > 4. prune_refs(): for each ref in the
ref_to_prune
> > > > list,
> > > >
> > > > call prune_ref():
> > > > a. Lock the loose reference using
> > > > lock_ref_sha1(),
> > > >
> > > > verifying that the recorded SHA1 is still valid
> > > >
> > > > b. If it is, unlink the loose reference
file
> > > >
> > > > (otherwise, leave it untouched)
> > > >
> > > > c. Release the lock on the loose reference
> > >
> > > To be honest, I suspect I missed something obvious
> > > because this seems almost too simple to work. I
am
> > > ashamed that it took me so long to come up with
(of
> > > course, I will be even more ashamed :( when it is
> > > shown to be flawed!) This scheme also feels
> > > extensible. if there are no obvious flaws in it, I
> > > will try to post solutions for ref packing and for
> > > multiple repository/ref transactions also soon.
> > >
> > > I welcome any comments/criticisms,
> > >
> > > -Martin
> > > --
> > > To unsubscribe from this list: send the line
> > > "unsubscribe git" in the body of a message to
> > > majordomo@vger.kernel.org More majordomo info at
> > > http://vger.kernel.org/majordomo-info.html
> >
> > --
> > To unsubscribe from this list: send the line
> > "unsubscribe git" in the body of a message to
> > majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: Proposal for git stash rename
From: Micheil Smith @ 2013-01-04 18:25 UTC (permalink / raw)
To: git
In-Reply-To: <20100620111112.GH24805@occam.hewgill.net>
Greg Hewgill <greg <at> hewgill.com> writes:
>
> On Sun, Jun 20, 2010 at 10:54:43AM +0000, ??var Arnfj??r?? Bjarmason wrote:
> > It's good to post a WIP PATCH even if it needs cleanup, just as a
> > point for further discussion.
>
> Thanks, point taken. WIP patch follows.
>
> This patch implements a "git stash rename" using a new
> "git reflog update" command that updates the message associated
> with a reflog entry.
> ---
> [--snip--]
Hi,
I note that this proposal is now two years old. A work in progress patch was
requested, however, after one was given this thread ended. I'm also finding
a need for this feature;
Not to try and bump an old thread, but what's the best way to land this?
– Micheil Smith
@miksago
^ permalink raw reply
* [PATCH] wincred: improve compatibility with windows versions
From: Karsten Blees @ 2013-01-04 20:28 UTC (permalink / raw)
To: git; +Cc: msysgit, kusmabite, Jeff King
On WinXP, the windows credential helper doesn't work at all (due to missing
Cred[Un]PackAuthenticationBuffer APIs). On Win7, the credential format used
by wincred is incompatible with native Windows tools (such as the control
panel applet or 'cmdkey.exe /generic'). These Windows tools only set the
TargetName, UserName and CredentialBlob members of the CREDENTIAL
structure (where CredentialBlob is the UTF-16-encoded password).
Remove the unnecessary packing / unpacking of the password, along with the
related API definitions, for compatibility with Windows XP.
Don't use CREDENTIAL_ATTRIBUTEs to identify credentials for compatibility
with Windows credential manager tools. Parse the protocol, username, host
and path fields from the credential's target name instead.
While we're at it, optionally accept CRLF (instead of LF only) to simplify
debugging in bash / cmd.
Signed-off-by: Karsten Blees <blees@dcon.de>
---
Hi,
I tried the windows credential helper yesterday and found that it doesn't work on XP at all, and doesn't work properly in combination with Win7 credential manager tools. So here's a patch that reduces it to the functionality provided / expected by Windows.
@Erik, Jeff: Please let me know if I'm missing something and the use of Cred[Un]PackAuthenticationBuffer or CREDENTIAL_ATTRIBUTEs actually served a useful purpose.
Cheers,
Karsten
Also available here:
https://github.com/kblees/git/tree/kb/improve-wincred-compatibility
git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility
---
.../credential/wincred/git-credential-wincred.c | 179 ++++++---------------
1 file changed, 53 insertions(+), 126 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index cbaec5f..3464080 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -9,6 +9,8 @@
/* common helpers */
+#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
+
static void die(const char *err, ...)
{
char msg[4096];
@@ -30,14 +32,6 @@ static void *xmalloc(size_t size)
return ret;
}
-static char *xstrdup(const char *str)
-{
- char *ret = strdup(str);
- if (!ret)
- die("Out of memory");
- return ret;
-}
-
/* MinGW doesn't have wincred.h, so we need to define stuff */
typedef struct _CREDENTIAL_ATTRIBUTEW {
@@ -67,20 +61,14 @@ typedef struct _CREDENTIALW {
#define CRED_MAX_ATTRIBUTES 64
typedef BOOL (WINAPI *CredWriteWT)(PCREDENTIALW, DWORD);
-typedef BOOL (WINAPI *CredUnPackAuthenticationBufferWT)(DWORD, PVOID, DWORD,
- LPWSTR, DWORD *, LPWSTR, DWORD *, LPWSTR, DWORD *);
typedef BOOL (WINAPI *CredEnumerateWT)(LPCWSTR, DWORD, DWORD *,
PCREDENTIALW **);
-typedef BOOL (WINAPI *CredPackAuthenticationBufferWT)(DWORD, LPWSTR, LPWSTR,
- PBYTE, DWORD *);
typedef VOID (WINAPI *CredFreeT)(PVOID);
typedef BOOL (WINAPI *CredDeleteWT)(LPCWSTR, DWORD, DWORD);
static HMODULE advapi, credui;
static CredWriteWT CredWriteW;
-static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW;
static CredEnumerateWT CredEnumerateW;
-static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
static CredFreeT CredFree;
static CredDeleteWT CredDeleteW;
@@ -88,74 +76,64 @@ static void load_cred_funcs(void)
{
/* load DLLs */
advapi = LoadLibrary("advapi32.dll");
- credui = LoadLibrary("credui.dll");
- if (!advapi || !credui)
+ if (!advapi)
die("failed to load DLLs");
/* get function pointers */
CredWriteW = (CredWriteWT)GetProcAddress(advapi, "CredWriteW");
- CredUnPackAuthenticationBufferW = (CredUnPackAuthenticationBufferWT)
- GetProcAddress(credui, "CredUnPackAuthenticationBufferW");
CredEnumerateW = (CredEnumerateWT)GetProcAddress(advapi,
"CredEnumerateW");
- CredPackAuthenticationBufferW = (CredPackAuthenticationBufferWT)
- GetProcAddress(credui, "CredPackAuthenticationBufferW");
CredFree = (CredFreeT)GetProcAddress(advapi, "CredFree");
CredDeleteW = (CredDeleteWT)GetProcAddress(advapi, "CredDeleteW");
- if (!CredWriteW || !CredUnPackAuthenticationBufferW ||
- !CredEnumerateW || !CredPackAuthenticationBufferW || !CredFree ||
- !CredDeleteW)
+ if (!CredWriteW || !CredEnumerateW || !CredFree || !CredDeleteW)
die("failed to load functions");
}
-static char target_buf[1024];
-static char *protocol, *host, *path, *username;
-static WCHAR *wusername, *password, *target;
+static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
-static void write_item(const char *what, WCHAR *wbuf)
+static void write_item(const char *what, LPCWSTR wbuf, int wlen)
{
char *buf;
- int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, NULL, 0, NULL,
+ int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL,
FALSE);
buf = xmalloc(len);
- if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, buf, len, NULL, FALSE))
+ if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, buf, len, NULL, FALSE))
die("WideCharToMultiByte failed!");
printf("%s=", what);
- fwrite(buf, 1, len - 1, stdout);
+ fwrite(buf, 1, len, stdout);
putchar('\n');
free(buf);
}
-static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword,
- const char *want)
+static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
{
- int i;
- if (!want)
- return 1;
-
- for (i = 0; i < cred->AttributeCount; ++i)
- if (!wcscmp(cred->Attributes[i].Keyword, keyword))
- return !strcmp((const char *)cred->Attributes[i].Value,
- want);
-
- return 0; /* not found */
+ LPCWSTR start = *ptarget;
+ LPCWSTR end = *delim ? wcsstr(start, delim) : start + wcslen(start);
+ int len = end ? end - start : wcslen(start);
+ /* update ptarget if we either found a delimiter or need a match */
+ if (end || want)
+ *ptarget = end ? end + wcslen(delim) : start + len;
+ return !want || (!wcsncmp(want, start, len) && !want[len]);
}
static int match_cred(const CREDENTIALW *cred)
{
- return (!wusername || !wcscmp(wusername, cred->UserName)) &&
- match_attr(cred, L"git_protocol", protocol) &&
- match_attr(cred, L"git_host", host) &&
- match_attr(cred, L"git_path", path);
+ LPCWSTR target = cred->TargetName;
+ if (wusername && wcscmp(wusername, cred->UserName))
+ return 0;
+
+ return match_part(&target, L"git", L":") &&
+ match_part(&target, protocol, L"://") &&
+ match_part(&target, wusername, L"@") &&
+ match_part(&target, host, L"/") &&
+ match_part(&target, path, L"");
}
static void get_credential(void)
{
- WCHAR *user_buf, *pass_buf;
- DWORD user_buf_size = 0, pass_buf_size = 0;
- CREDENTIALW **creds, *cred = NULL;
+ CREDENTIALW **creds;
DWORD num_creds;
int i;
@@ -165,44 +143,15 @@ static void get_credential(void)
/* search for the first credential that matches username */
for (i = 0; i < num_creds; ++i)
if (match_cred(creds[i])) {
- cred = creds[i];
+ write_item("username", creds[i]->UserName,
+ wcslen(creds[i]->UserName));
+ write_item("password",
+ (LPCWSTR)creds[i]->CredentialBlob,
+ creds[i]->CredentialBlobSize / sizeof(WCHAR));
break;
}
- if (!cred)
- return;
-
- CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
- cred->CredentialBlobSize, NULL, &user_buf_size, NULL, NULL,
- NULL, &pass_buf_size);
-
- user_buf = xmalloc(user_buf_size * sizeof(WCHAR));
- pass_buf = xmalloc(pass_buf_size * sizeof(WCHAR));
-
- if (!CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
- cred->CredentialBlobSize, user_buf, &user_buf_size, NULL, NULL,
- pass_buf, &pass_buf_size))
- die("CredUnPackAuthenticationBuffer failed");
CredFree(creds);
-
- /* zero-terminate (sizes include zero-termination) */
- user_buf[user_buf_size - 1] = L'\0';
- pass_buf[pass_buf_size - 1] = L'\0';
-
- write_item("username", user_buf);
- write_item("password", pass_buf);
-
- free(user_buf);
- free(pass_buf);
-}
-
-static void write_attr(CREDENTIAL_ATTRIBUTEW *attr, const WCHAR *keyword,
- const char *value)
-{
- attr->Keyword = (LPWSTR)keyword;
- attr->Flags = 0;
- attr->ValueSize = strlen(value) + 1; /* store zero-termination */
- attr->Value = (LPBYTE)value;
}
static void store_credential(void)
@@ -215,40 +164,18 @@ static void store_credential(void)
if (!wusername || !password)
return;
- /* query buffer size */
- CredPackAuthenticationBufferW(0, wusername, password,
- NULL, &auth_buf_size);
-
- auth_buf = xmalloc(auth_buf_size);
-
- if (!CredPackAuthenticationBufferW(0, wusername, password,
- auth_buf, &auth_buf_size))
- die("CredPackAuthenticationBuffer failed");
-
cred.Flags = 0;
cred.Type = CRED_TYPE_GENERIC;
cred.TargetName = target;
cred.Comment = L"saved by git-credential-wincred";
- cred.CredentialBlobSize = auth_buf_size;
- cred.CredentialBlob = auth_buf;
+ cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
+ cred.CredentialBlob = (LPVOID)password;
cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
- cred.AttributeCount = 1;
- cred.Attributes = attrs;
+ cred.AttributeCount = 0;
+ cred.Attributes = NULL;
cred.TargetAlias = NULL;
cred.UserName = wusername;
- write_attr(attrs, L"git_protocol", protocol);
-
- if (host) {
- write_attr(attrs + cred.AttributeCount, L"git_host", host);
- cred.AttributeCount++;
- }
-
- if (path) {
- write_attr(attrs + cred.AttributeCount, L"git_path", path);
- cred.AttributeCount++;
- }
-
if (!CredWriteW(&cred, 0))
die("CredWrite failed");
}
@@ -284,10 +211,13 @@ static void read_credential(void)
while (fgets(buf, sizeof(buf), stdin)) {
char *v;
+ int len = strlen(buf);
+ /* strip trailing CR / LF */
+ while (len && strchr("\r\n", buf[len - 1]))
+ buf[--len] = 0;
- if (!strcmp(buf, "\n"))
+ if (!*buf)
break;
- buf[strlen(buf)-1] = '\0';
v = strchr(buf, '=');
if (!v)
@@ -295,13 +225,12 @@ static void read_credential(void)
*v++ = '\0';
if (!strcmp(buf, "protocol"))
- protocol = xstrdup(v);
+ protocol = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "host"))
- host = xstrdup(v);
+ host = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "path"))
- path = xstrdup(v);
+ path = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "username")) {
- username = xstrdup(v);
wusername = utf8_to_utf16_dup(v);
} else if (!strcmp(buf, "password"))
password = utf8_to_utf16_dup(v);
@@ -330,22 +259,20 @@ int main(int argc, char *argv[])
return 0;
/* prepare 'target', the unique key for the credential */
- strncat(target_buf, "git:", sizeof(target_buf));
- strncat(target_buf, protocol, sizeof(target_buf));
- strncat(target_buf, "://", sizeof(target_buf));
- if (username) {
- strncat(target_buf, username, sizeof(target_buf));
- strncat(target_buf, "@", sizeof(target_buf));
+ wcscpy(target, L"git:");
+ wcsncat(target, protocol, ARRAY_SIZE(target));
+ wcsncat(target, L"://", ARRAY_SIZE(target));
+ if (wusername) {
+ wcsncat(target, wusername, ARRAY_SIZE(target));
+ wcsncat(target, L"@", ARRAY_SIZE(target));
}
if (host)
- strncat(target_buf, host, sizeof(target_buf));
+ wcsncat(target, host, ARRAY_SIZE(target));
if (path) {
- strncat(target_buf, "/", sizeof(target_buf));
- strncat(target_buf, path, sizeof(target_buf));
+ wcsncat(target, L"/", ARRAY_SIZE(target));
+ wcsncat(target, path, ARRAY_SIZE(target));
}
- target = utf8_to_utf16_dup(target_buf);
-
if (!strcmp(argv[1], "get"))
get_credential();
else if (!strcmp(argv[1], "store"))
--
1.8.0.msysgit.0.4.g4e40dea
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply related
* [PATCH] SubmittingPatches: Document how to request a patch review tag
From: Jason Holden @ 2013-01-04 20:58 UTC (permalink / raw)
To: gitster; +Cc: git, Jason Holden
In-Reply-To: <7vy5gb33f9.fsf@alter.siamese.dyndns.org>
Document the preferred way a developer should request to have their
Acked-by/Tested-by/Reviewed-by tag to a patch series under discussion
Signed-off-by: Jason Holden <jason.k.holden.swdev@gmail.com>
---
Junio,
I was ready to add my Reviewed-by to this patch series, but I wasn't sure if
I should email just you the patch author (to cut down on overall list traffic)
or both you and the list. If all reviewed-by/acked-by/tested-by traffic
should go via the email list I think this patch would be helpful, as I
wasn't quite sure how wide of a distribution list to use for my
"Reviewed-by" email.
A very similiar question was asked previously in:
http://thread.gmane.org/gmane.comp.version-control.git/185564/focus=185570
This will apply on top of your last tweak to SubmittingPatches
Please add my reviewed-by to the rest of the patches in this series.
-Jason
Documentation/SubmittingPatches | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index f6276ff..80001c9 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -268,6 +268,11 @@ If you like, you can put extra tags at the end:
4. "Tested-by:" is used to indicate that the person applied the patch
and found it to have the desired effect.
+If you are a reviewer and wish to add your Acked-by/Reviewed-by/Tested-by tag
+to a patch series under discussion (after having reviewed it or tested it
+of course!), reply to the author of the patch series, cc'ing the git mailing
+list.
+
You can also create your own tag or use one that's in common usage
such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
--
1.8.1.rc3.28.g0ab5d1f
^ permalink raw reply related
* [BUG] git submodule update is not fail safe
From: Manlio Perillo @ 2013-01-04 20:53 UTC (permalink / raw)
To: git
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi.
My network connection at times is rather unstable, so I can experience
all sort of network problems.
Today I tried to clone the qemu repository, and then to update all
submodules.
I'm using git from a recent master (790c83 - 14 December).
This is what happened:
$ git submodule update --init pixman
Submodule 'pixman' (git://anongit.freedesktop.org/pixman) registered for
path 'pixman'
Cloning into 'pixman'...
fatal: Unable to look up anongit.freedesktop.org (port 9418) (Name or
service not known)
Clone of 'git://anongit.freedesktop.org/pixman' into submodule path
'pixman' failed
$ git submodule update --init
Submodule 'roms/SLOF' (git://git.qemu.org/SLOF.git) registered for path
'roms/SLOF'
Submodule 'roms/ipxe' (git://git.qemu.org/ipxe.git) registered for path
'roms/ipxe'
Submodule 'roms/openbios' (git://git.qemu.org/openbios.git) registered
for path 'roms/openbios'
Submodule 'roms/qemu-palcode' (git://repo.or.cz/qemu-palcode.git)
registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (git://git.qemu.org/seabios.git/) registered
for path 'roms/seabios'
Submodule 'roms/sgabios' (git://git.qemu.org/sgabios.git) registered for
path 'roms/sgabios'
Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
for path 'roms/vgabios'
fatal: unable to connect to anongit.freedesktop.org:
anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
Unable to fetch in submodule path 'pixman'
$ git submodule update --init
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'
The problem is easy to solve: manually remove the pixman directory;
however IMHO git submodule update should not fail this way since it may
confuse the user.
I'm sorry for not sending a patch.
Regards Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDnQUUACgkQscQJ24LbaUSgVACglJjFxB51VINOCe9Z39/XEEUH
6+QAnAwdQerBSjVSS1/3eNXSBfnR0yba
=eOJT
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v3 11/19] dir.c: use a single struct exclude_list per source of excludes
From: Junio C Hamano @ 2013-01-04 21:03 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <1356575558-2674-12-git-send-email-git@adamspiers.org>
Adam Spiers <git@adamspiers.org> writes:
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 0c7b3d0..bd18b88 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> if (!ignored)
> setup_standard_excludes(&dir);
>
> + add_exclude_list(&dir, EXC_CMDL);
> for (i = 0; i < exclude_list.nr; i++)
> add_exclude(exclude_list.items[i].string, "", 0,
> - &dir.exclude_list[EXC_CMDL]);
> + &dir.exclude_list_groups[EXC_CMDL].ary[0]);
This looks somewhat ugly for two reasons.
* The abstraction add_exclude() offers to its callers is just to
let them add one pattern to the list of patterns for the kind
(here, EXC_CMDL); why should they care about .ary[0] part? Are
there cases any sane caller (not the implementation of the
exclude_list_group machinery e.g. add_excludes_from_... function)
may want to call it with .ary[1]? I do not think of any.
Shouldn't the public API function add_exclude() take a pointer to
the list group itself?
* When naming an array of things, we tend to prefer naming it
type thing[count]
so that the second element can be called "thing[2]" and not
"things[2]". dir.exclude_list_group[EXC_CMDL] reads beter.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index ef7f99a..c448e06 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -420,10 +420,10 @@ static int option_parse_z(const struct option *opt,
> static int option_parse_exclude(const struct option *opt,
> const char *arg, int unset)
> {
> - struct exclude_list *list = opt->value;
> + struct exclude_list_group *group = opt->value;
>
> exc_given = 1;
> - add_exclude(arg, "", 0, list);
> + add_exclude(arg, "", 0, &group->ary[0]);
This is another example where the caller would wish to be able to say
add_exclude(arg, "", 0, group);
instead.
^ permalink raw reply
* Re: as/check-ignore (was Re: What's cooking in git.git (Jan 2013, #02; Thu, 3))
From: Junio C Hamano @ 2013-01-04 21:13 UTC (permalink / raw)
To: Adam Spiers; +Cc: git
In-Reply-To: <CAOkDyE-f-8XZAzWrQgh_DG=fZctqBFXqpog-FSDU_yeXfwWTwA@mail.gmail.com>
Adam Spiers <git@adamspiers.org> writes:
> On Thu, Jan 3, 2013 at 7:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> * as/check-ignore (2012-12-28) 19 commits
>> - Add git-check-ignore sub-command
>> - setup.c: document get_pathspec()
>> - pathspec.c: extract new validate_path() for reuse
>> - pathspec.c: move reusable code from builtin/add.c
>> - add.c: remove unused argument from validate_pathspec()
>> - add.c: refactor treat_gitlinks()
>> - dir.c: provide clear_directory() for reclaiming dir_struct memory
>> - dir.c: keep track of where patterns came from
>> - dir.c: use a single struct exclude_list per source of excludes
>> - dir.c: rename free_excludes() to clear_exclude_list()
>> - dir.c: refactor is_path_excluded()
>> - dir.c: refactor is_excluded()
>> - dir.c: refactor is_excluded_from_list()
>> - dir.c: rename excluded() to is_excluded()
>> - dir.c: rename excluded_from_list() to is_excluded_from_list()
>> - dir.c: rename path_excluded() to is_path_excluded()
>> - dir.c: rename cryptic 'which' variable to more consistent name
>> - Improve documentation and comments regarding directory traversal API
>> - api-directory-listing.txt: update to match code
>>
>> Rerolled. The early parts looked mostly fine; we may want to split
>> this into two topics and have the early half graduate sooner.
>
> Sounds good to me. As already mentioned, I have the v4 series ready
> and it addresses all issues already voiced in v3, but I have postponed
> submitting it as per your request. Please let me know when and how to
> proceed, thanks!
I was planning to add a new "as/dir-c-cleanup" topic that leads to
f619881 (dir.c: rename free_excludes() to clear_exclude_list(),
2012-12-27), and leave the remainder in this series. I think the
earlier parts of this series up to that point should go 'next' now.
^ permalink raw reply
* RE: [PATCH v4] git-completion.bash: add support for path completion
From: Marc Khouzam @ 2013-01-04 21:25 UTC (permalink / raw)
To: 'Manlio Perillo', 'git@vger.kernel.org'
Cc: 'szeder@ira.uka.de', 'felipe.contreras@gmail.com'
In-Reply-To: <1356108872-5881-1-git-send-email-manlio.perillo@gmail.com>
> -----Original Message-----
> From: git-owner@vger.kernel.org
> [mailto:git-owner@vger.kernel.org] On Behalf Of Manlio Perillo
> Sent: Friday, December 21, 2012 11:55 AM
> To: git@vger.kernel.org
> Cc: szeder@ira.uka.de; felipe.contreras@gmail.com; Manlio Perillo
> Subject: [PATCH v4] git-completion.bash: add support for path
> completion
>
> The git-completion.bash script did not implemented full, git aware,
> support to complete paths, for git commands that operate on
> files within
> the current working directory or the index.
I think this is a great improvement! Very nice.
I've been playing with it but I'm not getting the expected
behavior when I cd to a sub-directory. Instead I get all files
in the repo as proposals. I'm trying it in the git git-repository
itself. Here is a sample:
> git status
# On branch pu
nothing to commit, working directory clean
> source contrib/completion/git-completion.bash
> touch contrib/test1
> touch contrib/test2
> git status
# On branch pu
# Untracked files:
# (use "git add <file>..." to include in what will be committed)
#
# contrib/test1
# contrib/test2
nothing added to commit but untracked files present (use "git add" to track)
> git add <TAB> # this works as expected and I get: contrib/test1 contrib/test2
> cd contrib/
> git add <TAB>
Display all 387 possibilities? (y or n) # That is not right. Shouldn't I get
# the same two files only?
Maybe I mis-understood what should happen?
Besides that, without looking at the patch in detail, I put just a couple
of minor points below.
> As an example:
>
> git add <TAB>
>
> will suggest all files in the current working directory, including
> ignored files and files that have not been modified.
>
> Support path completion, for git commands where the
> non-option arguments
> always refer to paths within the current working directory or
> the index, as the follow:
s/as the follow/as follows/
> * the path completion for the "git rm" and "git ls-files"
> commands will suggest all cached files.
>
> * the path completion for the "git add" command will suggest all
> untracked and modified files. Ignored files are excluded.
>
> * the path completion for the "git clean" command will suggest all
> untracked files. Ignored files are excluded.
>
> * the path completion for the "git mv" command will suggest all cached
> files when expanding the first argument, and all untracked
> and cached
> files for subsequent arguments. In the latter case, empty
> directories
> are included and ignored files are excluded.
>
> * the path completion for the "git commit" command will suggest all
> files that have been modified from the HEAD, if HEAD
> exists, otherwise
> it will suggest all cached files.
>
> For all affected commands, completion will always stop at directory
> boundary. Only standard ignored files are excluded, using the
> --exclude-standard option of the ls-files command.
>
> Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
> ---
>
> Changes from version 3:
>
> * Fixed quoting issues
> * Fixed default parameters handling
> * Fixed a typo in the commit message: the affected
> command was ls-files,
> not ls-tree.
> * Fixed incorrect behavior when expanding a path in "git commit"
> command, for a newly created repository (when HEAD does not
> exists).
> * Make sure to always execute git commands with stderr
> redirected to
> /dev/null.
> * Improved path completion for the git mv command.
> This required a small refactorization of the __git_index_files
> function, in order to support multiple options for ls-files.
>
> contrib/completion/git-completion.bash | 140
> +++++++++++++++++++++++++++++----
> 1 file changed, 124 insertions(+), 16 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 0b77eb1..c8c6464 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -13,6 +13,7 @@
> # *) .git/remotes file names
> # *) git 'subcommands'
> # *) tree paths within 'ref:path/to/file' expressions
> +# *) file paths within current working directory and index
> # *) common --long-options
> #
> # To use these routines:
> @@ -233,6 +234,62 @@ __gitcomp_nl ()
> COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" --
> "${3-$cur}"))
> }
>
> +# Process path list returned by "ls-files" and "diff-index
> --name-only"
> +# commands, in order to list only file names relative to a specified
> +# directory, and append a slash to directory names.
> +# It accepts 1 optional argument: a directory path. The
> path must have
> +# a trailing slash.
> +__git_index_file_list_filter ()
> +{
> + local pfx="${1-}" offset=${#pfx} path
> +
> + while read -r path; do
> + path="${path:$offset}"
> +
> + case "$path" in
> + ?*/*) echo "${path%%/*}/" ;;
> + *) echo $path ;;
> + esac
> + done
> +}
> +
> +# __git_index_files accepts 1 or 2 arguments:
> +# 1: Options to pass to ls-files (required).
> +# Supported options are --cached, --modified, --deleted, --others,
> +# and --directory.
> +# 2: A directory path (optional).
> +# If provided, only files within the specified directory
> are listed.
> +# Sub directories are never recursed. Path must have a trailing
> +# slash.
> +__git_index_files ()
> +{
> + local dir="$(__gitdir)"
> +
> + if [ -d "$dir" ]; then
> + # NOTE: $1 is not quoted in order to support
> multiple options
> + git --git-dir="$dir" ls-files
> --exclude-standard $1 ${2+"$2"} 2>/dev/null |
> + __git_index_file_list_filter ${2+"$2"} |
> + uniq
Will the output piped to uniq always be sorted? It has to be.
If it is not, you must use
sort | uniq
> + fi
> +}
> +
> +# __git_diff_index_files accepts 1 or 2 arguments:
> +# 1) The id of a tree object.
> +# 2) A directory path (optional).
> +# If provided, only files within the specified directory
> are listed.
> +# Sub directories are never recursed. Path must have a trailing
> +# slash.
> +__git_diff_index_files ()
> +{
> + local dir="$(__gitdir)"
> +
> + if [ -d "$dir" ]; then
> + git --git-dir="$dir" diff-index --name-only
> "$1" 2>/dev/null |
> + __git_index_file_list_filter ${2+"$2"} |
> + uniq
Will the output piped to uniq always be sorted? It has to be.
If it is not, you must use
sort | uniq
> + fi
> +}
> +
> __git_heads ()
> {
> local dir="$(__gitdir)"
> @@ -430,6 +487,46 @@ __git_complete_revlist_file ()
> }
>
>
> +# __git_complete_index_file requires 1 argument: the options
> to pass to
> +# ls-file
> +__git_complete_index_file ()
> +{
> + local pfx cur_="$cur"
> +
> + case "$cur_" in
> + ?*/*)
> + pfx="${cur_%/*}"
> + cur_="${cur_##*/}"
> + pfx="${pfx}/"
> +
> + __gitcomp_nl "$(__git_index_files "$1" "$pfx")"
> "$pfx" "$cur_" ""
> + ;;
> + *)
> + __gitcomp_nl "$(__git_index_files "$1")" "" "$cur_" ""
> + ;;
> + esac
> +}
> +
> +# __git_complete_diff_index_file requires 1 argument: the id
> of a tree
> +# object
> +__git_complete_diff_index_file ()
> +{
> + local pfx cur_="$cur"
> +
> + case "$cur_" in
> + ?*/*)
> + pfx="${cur_%/*}"
> + cur_="${cur_##*/}"
> + pfx="${pfx}/"
> +
> + __gitcomp_nl "$(__git_diff_index_files "$1"
> "$pfx")" "$pfx" "$cur_" ""
> + ;;
> + *)
> + __gitcomp_nl "$(__git_diff_index_files "$1")"
> "" "$cur_" ""
> + ;;
> + esac
> +}
> +
> __git_complete_file ()
> {
> __git_complete_revlist_file
> @@ -770,8 +867,6 @@ _git_apply ()
>
> _git_add ()
> {
> - __git_has_doubledash && return
> -
> case "$cur" in
> --*)
> __gitcomp "
> @@ -780,7 +875,9 @@ _git_add ()
> "
> return
> esac
> - COMPREPLY=()
> +
> + # XXX should we check for --update and --all options ?
> + __git_complete_index_file "--others --modified"
> }
>
> _git_archive ()
> @@ -930,15 +1027,15 @@ _git_cherry_pick ()
>
> _git_clean ()
> {
> - __git_has_doubledash && return
> -
> case "$cur" in
> --*)
> __gitcomp "--dry-run --quiet"
> return
> ;;
> esac
> - COMPREPLY=()
> +
> + # XXX should we check for -x option ?
> + __git_complete_index_file "--others"
> }
>
> _git_clone ()
> @@ -969,8 +1066,6 @@ _git_clone ()
>
> _git_commit ()
> {
> - __git_has_doubledash && return
> -
> case "$cur" in
> --cleanup=*)
> __gitcomp "default strip verbatim whitespace
> @@ -998,7 +1093,13 @@ _git_commit ()
> "
> return
> esac
> - COMPREPLY=()
> +
> + if git rev-parse --verify --quiet HEAD 1>/dev/null; then
> + __git_complete_diff_index_file "HEAD"
> + else
> + # This is the first commit
> + __git_complete_index_file "--cached"
> + fi
> }
>
> _git_describe ()
> @@ -1216,8 +1317,6 @@ _git_init ()
>
> _git_ls_files ()
> {
> - __git_has_doubledash && return
> -
> case "$cur" in
> --*)
> __gitcomp "--cached --deleted --modified
> --others --ignored
> @@ -1230,7 +1329,10 @@ _git_ls_files ()
> return
> ;;
> esac
> - COMPREPLY=()
> +
> + # XXX ignore options like --modified and always suggest
> all cached
> + # files.
> + __git_complete_index_file "--cached"
> }
>
> _git_ls_remote ()
> @@ -1362,7 +1464,14 @@ _git_mv ()
> return
> ;;
> esac
> - COMPREPLY=()
> +
> + if [ $cword -gt 2 ]; then
> + # We need to show both cached and untracked
> files (including
> + # empty directories) since this may not be the
> last argument.
> + __git_complete_index_file "--cached --others
> --directory"
> + else
> + __git_complete_index_file "--cached"
> + fi
> }
>
> _git_name_rev ()
> @@ -2068,15 +2177,14 @@ _git_revert ()
>
> _git_rm ()
> {
> - __git_has_doubledash && return
> -
> case "$cur" in
> --*)
> __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
> return
> ;;
> esac
> - COMPREPLY=()
> +
> + __git_complete_index_file "--cached"
> }
>
> _git_shortlog ()
> --
> 1.8.1.rc1.18.g9db0d25
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases
From: Jeff King @ 2013-01-04 21:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Bart Trojanowski
In-Reply-To: <50E70976.5040001@kdbg.org>
On Fri, Jan 04, 2013 at 05:55:18PM +0100, Johannes Sixt wrote:
> Am 04.01.2013 13:47, schrieb Jeff King:
> > I have two reservations with this patch:
> >
> > 1. We are ignoring SIGPIPE all the time. For an alias that is calling
> > "log", that is fine. But if pack-objects dies on the server side,
> > seeing that it died from SIGPIPE is useful data, and we are
> > squelching that. Maybe callers of run-command should have to pass
> > an "ignore SIGPIPE" flag?
>
> I am of two minds. On the one hand, losing useful debugging information
> is not something we should do lightly. On the other hand, the message is
> really noise most of the time, even on servers: when pack-objects dies
> on the server side, it is most likely due to a connection that breaks
> (voluntarily or involuntarily) half-way during a transfer, and is
> presumably a frequent event, and as such not worth noting most of the time.
Yeah. I'd mostly be worried about a case where pack-objects prints
nothing (because it dies due to pipe), and then the outer process is not
sufficiently verbose (it just says something like "pack-objects died
abnormally", and the user is left scratching their head. I.e., it _is_
uninteresting, but because we are too silent, the user does not even
know it is uninteresting.
Pack-objects is already careful to check all of its writes. I really
think it would be fine to just ignore SIGPIPE, and then it would produce
a useful error message on EPIPE. The downside is that if we accidentally
have an unchecked call, we won't notice the error (we'll probably notice
it later, but we might continue uselessly spewing data in the meantime).
Perhaps we should catch SIGPIPE in such programs and print an error
message.
> > 2. The die_errno in handle_alias is definitely wrong. Even if we want
> > to print a message for signal death, showing errno is bogus unless
> > the return value was -1. But is it the right thing to just pass the
> > negative value straight to exit()? It works, but it is depending on
> > the fact that (unsigned char)(ret & 0xff) behaves in a certain way
> > (i.e., that we are on a twos-complement platform, and -13 becomes
> > 141). That is not strictly portable, but it is probably fine in
> > practice. I'd worry more about exit() doing something weird on
> > Windows.
>
> It did something weird on Windows until we added this line to
> compat/mingw.h:
>
> #define exit(code) exit((code) & 0xff)
Ah, makes sense. I think that hunk of my patch is probably good, then.
-Peff
^ permalink raw reply
* Re: Lockless Refs?
From: Junio C Hamano @ 2013-01-04 21:28 UTC (permalink / raw)
To: Martin Fick; +Cc: Michael Haggerty, Shawn Pearce, Jeff King, git
In-Reply-To: <201301031652.44982.mfick@codeaurora.org>
Martin Fick <mfick@codeaurora.org> writes:
> Any thoughts on this idea? Is it flawed? I am trying to
> write it up in a more formal generalized manner and was
> hoping to get at least one "it seems sane" before I do.
The general impression I have been getting was that this isn't even
worth the effort and the resulting complexity of the code, given
Peff's observations earlier in the thread that ref update conflicts
and leftover locks are reasonably rare in practice. But perhaps I
has been mis-reading the discussion.
I also have this suspicion that if you really want to shoot for
multi-repository transactions in an massively scaled repository
hosting environment, you would rather want to not rely on hacks
based on filesystem semantics, but instead want to RPC with a
dedicated "ref management service" that knows the transaction
semantics you want, but that could become a much larger change.
I dunno.
^ permalink raw reply
* Re: Proposal for git stash rename
From: Junio C Hamano @ 2013-01-04 21:40 UTC (permalink / raw)
To: Micheil Smith; +Cc: git
In-Reply-To: <loom.20130104T192132-16@post.gmane.org>
Micheil Smith <micheil@brandedcode.com> writes:
>> This patch implements a "git stash rename" using a new
>> "git reflog update" command that updates the message associated
>> with a reflog entry.
> ...
> I note that this proposal is now two years old. A work in progress patch was
> requested, however, after one was given this thread ended. I'm also finding
> a need for this feature;
The whole point of reflog is that it is a mechanism to let users to
go safely back to the previous state, by using a file that is pretty
much append-only. It feels that a mechanism to "rewrite" one goes
completely against that principle, at least to me.
I have a feeling that "need" in "need for this feature" is a
misspelt "want", that occasional misspelling of the stash message
may give users awkward feelings when viewing "git stash list" output
but not severe enough to make them unable to identify which stash
entry holds which change, and that it is sufficient to pop and then
restash if a user *really* cares.
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: Document how to request a patch review tag
From: Junio C Hamano @ 2013-01-04 21:47 UTC (permalink / raw)
To: Jason Holden; +Cc: git
In-Reply-To: <1357333116-6971-1-git-send-email-jason.k.holden.swdev@gmail.com>
Jason Holden <jason.k.holden.swdev@gmail.com> writes:
> A very similiar question was asked previously in:
> http://thread.gmane.org/gmane.comp.version-control.git/185564/focus=185570
"Reviewed-by" is for those who are familiar with the part of the
system being touched to say "I reviewed this patch, it looks good",
and Michael indeed was involved in recent updates to the refs.c
infrastructure, so as he said in his message "it looks like I should",
it was the right thing to do.
I do not think Michael was asking if that was the standard _thing_
to do; I think the question was if there was a standard _way_
(perhaps a tool) to send such a "Reviewed-by:" line.
> This will apply on top of your last tweak to SubmittingPatches
>
> Please add my reviewed-by to the rest of the patches in this series.
I do not think you "own" anyting in SubmittingPatches document,
though; at least not yet.
^ permalink raw reply
* Re: [BUG] git submodule update is not fail safe
From: Junio C Hamano @ 2013-01-04 21:51 UTC (permalink / raw)
To: Jens Lehmann, Heiko Voigt; +Cc: git, Manlio Perillo, W. Trevor King
In-Reply-To: <50E74145.4020701@gmail.com>
Manlio Perillo <manlio.perillo@gmail.com> writes:
> $ git submodule update --init
> ...
> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
> for path 'roms/vgabios'
> fatal: unable to connect to anongit.freedesktop.org:
> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
>
> Unable to fetch in submodule path 'pixman'
>
> $ git submodule update --init
> fatal: Needed a single revision
> Unable to find current revision in submodule path 'pixman'
>
> The problem is easy to solve: manually remove the pixman directory;
> however IMHO git submodule update should not fail this way since it may
> confuse the user.
Sounds like a reasonable observation. Jens, Heiko, comments?
^ permalink raw reply
* Re: [PATCH] wincred: improve compatibility with windows versions
From: Erik Faye-Lund @ 2013-01-04 21:57 UTC (permalink / raw)
To: blees; +Cc: git, msysgit, Jeff King
In-Reply-To: <50E73B80.4070105@gmail.com>
On Fri, Jan 4, 2013 at 9:28 PM, Karsten Blees <karsten.blees@gmail.com> wrote:
> On WinXP, the windows credential helper doesn't work at all (due to missing
> Cred[Un]PackAuthenticationBuffer APIs). On Win7, the credential format used
> by wincred is incompatible with native Windows tools (such as the control
> panel applet or 'cmdkey.exe /generic'). These Windows tools only set the
> TargetName, UserName and CredentialBlob members of the CREDENTIAL
> structure (where CredentialBlob is the UTF-16-encoded password).
>
> Remove the unnecessary packing / unpacking of the password, along with the
> related API definitions, for compatibility with Windows XP.
>
> Don't use CREDENTIAL_ATTRIBUTEs to identify credentials for compatibility
> with Windows credential manager tools. Parse the protocol, username, host
> and path fields from the credential's target name instead.
>
> While we're at it, optionally accept CRLF (instead of LF only) to simplify
> debugging in bash / cmd.
>
> Signed-off-by: Karsten Blees <blees@dcon.de>
> ---
>
> Hi,
>
> I tried the windows credential helper yesterday and found that it doesn't work on XP at all, and doesn't work properly in combination with Win7 credential manager tools. So here's a patch that reduces it to the functionality provided / expected by Windows.
>
> @Erik, Jeff: Please let me know if I'm missing something and the use of Cred[Un]PackAuthenticationBuffer or CREDENTIAL_ATTRIBUTEs actually served a useful purpose.
>
The only reason why I used Cred[Un]PackAuthenticationBuffer, were that
I wasn't aware that it was possible any other way. I didn't even know
there was a Windows Credential Manager in Windows XP.
The credential attributes were because they were convenient, and I'm
not sure I understand what you mean about the Win7 credential manager
tools. I did test my code with it - in fact, it was a very useful tool
for debugging the helper.
Are you referring to the credentials not *looking* like normal
HTTP-credentials? If so, I simply didn't see that as a goal. Why would
it be? Compatibility with IE? We already lose that with our "git:"
prefix in the target, no? Perhaps we can lose the "git:"-prefix, and
gain IE-compatibility when the protocol matches?
But, if we do any of these changes, does this mean I will lose my
existing credentials? It's probably not a big deal, but it's worth a
mention, isn't it?
> @@ -67,20 +61,14 @@ typedef struct _CREDENTIALW {
> #define CRED_MAX_ATTRIBUTES 64
>
> typedef BOOL (WINAPI *CredWriteWT)(PCREDENTIALW, DWORD);
> -typedef BOOL (WINAPI *CredUnPackAuthenticationBufferWT)(DWORD, PVOID, DWORD,
> - LPWSTR, DWORD *, LPWSTR, DWORD *, LPWSTR, DWORD *);
> typedef BOOL (WINAPI *CredEnumerateWT)(LPCWSTR, DWORD, DWORD *,
> PCREDENTIALW **);
> -typedef BOOL (WINAPI *CredPackAuthenticationBufferWT)(DWORD, LPWSTR, LPWSTR,
> - PBYTE, DWORD *);
> typedef VOID (WINAPI *CredFreeT)(PVOID);
> typedef BOOL (WINAPI *CredDeleteWT)(LPCWSTR, DWORD, DWORD);
>
> static HMODULE advapi, credui;
Perhaps get rid of credui also?
> static CredWriteWT CredWriteW;
> -static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW;
> static CredEnumerateWT CredEnumerateW;
> -static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
> static CredFreeT CredFree;
> static CredDeleteWT CredDeleteW;
>
> @@ -88,74 +76,64 @@ static void load_cred_funcs(void)
> {
> /* load DLLs */
> advapi = LoadLibrary("advapi32.dll");
> - credui = LoadLibrary("credui.dll");
> - if (!advapi || !credui)
> + if (!advapi)
> die("failed to load DLLs");
It's not multiple DLLs any more, so perhaps "failed to load
advapi32.dll" instead?
> -static char target_buf[1024];
> -static char *protocol, *host, *path, *username;
> -static WCHAR *wusername, *password, *target;
> +static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
>
> -static void write_item(const char *what, WCHAR *wbuf)
> +static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> {
> char *buf;
> - int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, NULL, 0, NULL,
> + int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL,
> FALSE);
> buf = xmalloc(len);
>
> - if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, buf, len, NULL, FALSE))
> + if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, buf, len, NULL, FALSE))
> die("WideCharToMultiByte failed!");
>
> printf("%s=", what);
> - fwrite(buf, 1, len - 1, stdout);
> + fwrite(buf, 1, len, stdout);
> putchar('\n');
> free(buf);
> }
>
When the password-blob is simply UTF-16 encoded, are the
zero-termination not included? That's the reason for this change,
right?
> -static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword,
> - const char *want)
> +static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
> {
> - int i;
> - if (!want)
> - return 1;
> -
> - for (i = 0; i < cred->AttributeCount; ++i)
> - if (!wcscmp(cred->Attributes[i].Keyword, keyword))
> - return !strcmp((const char *)cred->Attributes[i].Value,
> - want);
> -
> - return 0; /* not found */
> + LPCWSTR start = *ptarget;
> + LPCWSTR end = *delim ? wcsstr(start, delim) : start + wcslen(start);
> + int len = end ? end - start : wcslen(start);
> + /* update ptarget if we either found a delimiter or need a match */
> + if (end || want)
> + *ptarget = end ? end + wcslen(delim) : start + len;
> + return !want || (!wcsncmp(want, start, len) && !want[len]);
> }
>
I'm a bit tired now, but I'm having a hard time reading this code.
Right now it seems it's a bit too ternary-expression heavy for my
taste, though.
> static int match_cred(const CREDENTIALW *cred)
> {
> - return (!wusername || !wcscmp(wusername, cred->UserName)) &&
> - match_attr(cred, L"git_protocol", protocol) &&
> - match_attr(cred, L"git_host", host) &&
> - match_attr(cred, L"git_path", path);
> + LPCWSTR target = cred->TargetName;
> + if (wusername && wcscmp(wusername, cred->UserName))
> + return 0;
> +
> + return match_part(&target, L"git", L":") &&
> + match_part(&target, protocol, L"://") &&
> + match_part(&target, wusername, L"@") &&
> + match_part(&target, host, L"/") &&
> + match_part(&target, path, L"");
> }
>
Ugh, it feels a bit wrong to store and verify the username twice. Do
we really have to?
The target needs to be unique, even if two different usernames are
stored for the same site under the same conditions. So probably. It
just doesn't feel quite right.
> @@ -165,44 +143,15 @@ static void get_credential(void)
> /* search for the first credential that matches username */
> for (i = 0; i < num_creds; ++i)
> if (match_cred(creds[i])) {
> - cred = creds[i];
> + write_item("username", creds[i]->UserName,
> + wcslen(creds[i]->UserName));
> + write_item("password",
> + (LPCWSTR)creds[i]->CredentialBlob,
> + creds[i]->CredentialBlobSize / sizeof(WCHAR));
> break;
> }
> - if (!cred)
> - return;
> -
> - CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
> - cred->CredentialBlobSize, NULL, &user_buf_size, NULL, NULL,
> - NULL, &pass_buf_size);
> -
> - user_buf = xmalloc(user_buf_size * sizeof(WCHAR));
> - pass_buf = xmalloc(pass_buf_size * sizeof(WCHAR));
> -
> - if (!CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
> - cred->CredentialBlobSize, user_buf, &user_buf_size, NULL, NULL,
> - pass_buf, &pass_buf_size))
> - die("CredUnPackAuthenticationBuffer failed");
>
> CredFree(creds);
> -
> - /* zero-terminate (sizes include zero-termination) */
> - user_buf[user_buf_size - 1] = L'\0';
> - pass_buf[pass_buf_size - 1] = L'\0';
> -
> - write_item("username", user_buf);
> - write_item("password", pass_buf);
> -
> - free(user_buf);
> - free(pass_buf);
> -}
Nice!
> -
> -static void write_attr(CREDENTIAL_ATTRIBUTEW *attr, const WCHAR *keyword,
> - const char *value)
> -{
> - attr->Keyword = (LPWSTR)keyword;
> - attr->Flags = 0;
> - attr->ValueSize = strlen(value) + 1; /* store zero-termination */
> - attr->Value = (LPBYTE)value;
> }
>
> static void store_credential(void)
> @@ -215,40 +164,18 @@ static void store_credential(void)
> if (!wusername || !password)
> return;
>
> - /* query buffer size */
> - CredPackAuthenticationBufferW(0, wusername, password,
> - NULL, &auth_buf_size);
> -
> - auth_buf = xmalloc(auth_buf_size);
> -
> - if (!CredPackAuthenticationBufferW(0, wusername, password,
> - auth_buf, &auth_buf_size))
> - die("CredPackAuthenticationBuffer failed");
> -
> cred.Flags = 0;
> cred.Type = CRED_TYPE_GENERIC;
> cred.TargetName = target;
> cred.Comment = L"saved by git-credential-wincred";
> - cred.CredentialBlobSize = auth_buf_size;
> - cred.CredentialBlob = auth_buf;
> + cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> + cred.CredentialBlob = (LPVOID)password;
> cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> - cred.AttributeCount = 1;
> - cred.Attributes = attrs;
> + cred.AttributeCount = 0;
> + cred.Attributes = NULL;
> cred.TargetAlias = NULL;
> cred.UserName = wusername;
>
> - write_attr(attrs, L"git_protocol", protocol);
> -
> - if (host) {
> - write_attr(attrs + cred.AttributeCount, L"git_host", host);
> - cred.AttributeCount++;
> - }
> -
> - if (path) {
> - write_attr(attrs + cred.AttributeCount, L"git_path", path);
> - cred.AttributeCount++;
> - }
> -
> if (!CredWriteW(&cred, 0))
> die("CredWrite failed");
> }
Nice.
> @@ -284,10 +211,13 @@ static void read_credential(void)
>
> while (fgets(buf, sizeof(buf), stdin)) {
> char *v;
> + int len = strlen(buf);
> + /* strip trailing CR / LF */
> + while (len && strchr("\r\n", buf[len - 1]))
> + buf[--len] = 0;
>
> - if (!strcmp(buf, "\n"))
> + if (!*buf)
> break;
> - buf[strlen(buf)-1] = '\0';
>
> v = strchr(buf, '=');
> if (!v)
I think this part deserves a separate patch, no?
> @@ -295,13 +225,12 @@ static void read_credential(void)
> *v++ = '\0';
>
> if (!strcmp(buf, "protocol"))
> - protocol = xstrdup(v);
> + protocol = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "host"))
> - host = xstrdup(v);
> + host = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "path"))
> - path = xstrdup(v);
> + path = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "username")) {
> - username = xstrdup(v);
> wusername = utf8_to_utf16_dup(v);
> } else if (!strcmp(buf, "password"))
> password = utf8_to_utf16_dup(v);
So, you need the strings as UTF-16 instead so you can match against them...
> @@ -330,22 +259,20 @@ int main(int argc, char *argv[])
> return 0;
>
> /* prepare 'target', the unique key for the credential */
> - strncat(target_buf, "git:", sizeof(target_buf));
> - strncat(target_buf, protocol, sizeof(target_buf));
> - strncat(target_buf, "://", sizeof(target_buf));
> - if (username) {
> - strncat(target_buf, username, sizeof(target_buf));
> - strncat(target_buf, "@", sizeof(target_buf));
> + wcscpy(target, L"git:");
> + wcsncat(target, protocol, ARRAY_SIZE(target));
> + wcsncat(target, L"://", ARRAY_SIZE(target));
> + if (wusername) {
> + wcsncat(target, wusername, ARRAY_SIZE(target));
> + wcsncat(target, L"@", ARRAY_SIZE(target));
> }
> if (host)
> - strncat(target_buf, host, sizeof(target_buf));
> + wcsncat(target, host, ARRAY_SIZE(target));
> if (path) {
> - strncat(target_buf, "/", sizeof(target_buf));
> - strncat(target_buf, path, sizeof(target_buf));
> + wcsncat(target, L"/", ARRAY_SIZE(target));
> + wcsncat(target, path, ARRAY_SIZE(target));
> }
>
> - target = utf8_to_utf16_dup(target_buf);
> -
...Which means that you are composing a UTF-16 target directly rather
than ASCII. Looks sane.
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox