* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-05 18:10 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <ab1e7ce9-1022-0c72-2f72-63e3b9182bc9@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Heh, that was the first version of the patch. However, I got to thinking
> about why --abbrev=7 was there in the first place; the only reason I
> could think of was to defeat local configuration to get a measure of
> reproducibility.
>
> Unfortunately, you can't get the 'auto' behaviour from --abbrev
> (on the pu branch):
>
> $ ./git describe --abbrev=-1
> v2.11.0-286-g109e8
> $ ./git describe --abbrev=0
> v2.11.0
> $ ./git describe
> v2.11.0-286-g109e8a99d
> $
What is the reason why the last one is undesirable? Is it because
the user may have core.abbrev set to some value in the configuration
and you want to override it to force "auto"?
I am not sure how rigid GIT-VERSION-GEN wants to be to countermand
such an explicit user preference (i.e. existing configuration).
> I did think about using '-c core.abbrev=auto',
Having said that, if countermanding end-user's configuration is
desireble, I agree that "-c core.abbrev=auto" is the way to do so.
> but that would depend on Junio's patch (nothing wrong with that,
> of course):
You caught me. I'll need to polish that into a usable shape soon
then. And that is orthogonal to the "does it make sense to force
'auto' in this context?" question.
^ permalink raw reply
* Re: [PATCH] clone,fetch: explain the shallow-clone option a little more clearly
From: Junio C Hamano @ 2016-12-05 18:28 UTC (permalink / raw)
To: Alex Henrie; +Cc: pclouds, git
In-Reply-To: <20161204220359.30807-1-alexhenrie24@gmail.com>
Alex Henrie <alexhenrie24@gmail.com> writes:
> "deepen by excluding" does not make sense because excluding a revision
> does not deepen a repository; it makes the repository more shallow.
I think that an intuitive way the feature should work may be:
- You started with "git fetch --depth=20" and then later say "I
have only a very short segment of the recent history, but I want
a history that dates back to v1.0" with "--shallow-exclude=v1.0".
In this case, you would be deepening.
- You instead started with "git fetch --depth=20000" that dated
back to v0.5. "--shallow-exclude=v1.0" you say today would mean
"I have very old cruft I no longer look at. I just want my
history lead back to v1.0 and no earlier". In such a case, you
indeed would be making the repository shallower.
I however offhand do not think the feature can be used to make the
repository shallower, and I agree your changes to the usage string
probably describe what the option does more correctly.
I however suspect that the feature is simply buggy and it would
eventually want to allow to shorten the history as well. At that
point we may want to work on the verb 'deepen' there, too.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> builtin/clone.c | 2 +-
> builtin/fetch.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 6c76a6e..e3cb808 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -99,7 +99,7 @@ static struct option builtin_clone_options[] = {
> OPT_STRING(0, "shallow-since", &option_since, N_("time"),
> N_("create a shallow clone since a specific time")),
> OPT_STRING_LIST(0, "shallow-exclude", &option_not, N_("revision"),
> - N_("deepen history of shallow clone by excluding rev")),
> + N_("deepen history of shallow clone, excluding rev")),
> OPT_BOOL(0, "single-branch", &option_single_branch,
> N_("clone only one branch, HEAD or --branch")),
> OPT_BOOL(0, "shallow-submodules", &option_shallow_submodules,
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b6a5597..fc74c84 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -122,7 +122,7 @@ static struct option builtin_fetch_options[] = {
> OPT_STRING(0, "shallow-since", &deepen_since, N_("time"),
> N_("deepen history of shallow repository based on time")),
> OPT_STRING_LIST(0, "shallow-exclude", &deepen_not, N_("revision"),
> - N_("deepen history of shallow clone by excluding rev")),
> + N_("deepen history of shallow clone, excluding rev")),
> OPT_INTEGER(0, "deepen", &deepen_relative,
> N_("deepen history of shallow clone")),
> { OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
^ permalink raw reply
* Re: [BUG] git gui can't commit multiple files
From: Timon @ 2016-12-05 18:36 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <20161204204057.32dnkjx6ixv3swez@gmail.com>
On 12/4/16, David Aguilar <davvid@gmail.com> wrote:
> On Sun, Dec 04, 2016 at 05:36:46PM +0100, Timon wrote:
>> This is a regression in git 2.11.0 (version 2.10.2 is fine).
>>
>> In git-gui I select multiple files in the Unstaged Changes (using
>> shift+click) and press ctrl+t to stage them. Then only one files gets
>> staged instead of all of the selected files.
>> The same happens when unstaging files.
>>
>> Git-cola also exhibits the same behavior. Although there I could stage
>> multiple files if I used a popup menu instead of the keyboard shortcut
>> (I'm guessing it goes through a different code path?).
>
> Can you elaborate a bit?
>
> I just tested git-cola with Git 2.11 and it worked fine for me.
> I selected several files and used the Ctrl+s hotkey to stage the
> selected files. They all got staged.
>
> If you have a test repo, or reproduction recipe, I'd be curious
> to try it out.
> --
> David
>
Can you try with git gui?
Though I guess it's probably specific to my distro or configuration.
I'm running 64bit gentoo with:
linux 4.8.12
gcc 5.4.0
glibc 2.23-r3
tk 8.6.6
gettext 0.19.8.1
openssl 1.0.2j
Not sure if that's helpful though.
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Junio C Hamano @ 2016-12-05 18:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612051126320.117539@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Seriously, do you really think it is a good idea to have
> git_config_get_value() *ignore* any value in .git/config?
When we do not know ".git/" directory we see is the repository we
were told to work on, then we should ignore ".git/config" file. So
allowing git_config_get_value() to _ignore_ ".git/config" before the
program calls setup_git_directory() does have its uses.
But I agree that "difftool.useBuiltin" that flips between two
implementations is a different use case than the above. Both
implementations eventually want to work on ".git/" and would
want to read from ".git/config".
> We need to fix this.
I have a feeling that "environment modifications that cannot be
undone" we used as the rationale in 73c2779f42 ("builtin-am:
implement skeletal builtin am", 2015-08-04) might be overly
pessimistic and in order to implement undo_setup_git_directory(),
all we need to do may just be matter of doing a chdir(2) back to
prefix and unsetting GIT_PREFIX environment, but I haven't looked
into details of the setup sequence recently.
^ permalink raw reply
* [PATCH] making real_path thread-safe
From: Brandon Williams @ 2016-12-05 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, jacob.keller
The goal of this patch is to make the function real_path thread-safe. To do,
the current logic which uses chdir() to do the symlink resolution needed to be
rewritten to perform the symlink resolution by hand.
I wanted to get the bulk of the work out there for review quickly so
technically this patch doesn't make real_path thread safe yet as it still uses
a static buffer to serve up the resultant path. On a future iteration I plan
on removing this and pushing ownership of the returned string to the callers.
For more information on why this change is desirable you can see the following
threads:
https://public-inbox.org/git/20161129010538.GA121643@google.com/
https://public-inbox.org/git/1480555714-186183-1-git-send-email-bmwill@google.com/
Brandon Williams (1):
real_path: make real_path thread-safe
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 122 insertions(+), 61 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, jacob.keller
In-Reply-To: <1480964316-99305-1-git-send-email-bmwill@google.com>
The current implementation of real_path uses chdir() in order to resolve
symlinks. Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread. Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 122 insertions(+), 61 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2825de8..6f546e0 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,38 @@ int is_directory(const char *path)
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+ if (path->len > 1) {
+ char *last_slash = find_last_dir_sep(path->buf);
+ strbuf_setlen(path, last_slash - path->buf);
+ }
+}
+
+/* gets the next component in 'remaining' and places it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+ char *start = NULL;
+ char *end = NULL;
+
+ strbuf_reset(next);
+
+ /* look for the next component */
+ /* Skip sequences of multiple path-separators */
+ for (start = remaining->buf; is_dir_sep(*start); start++)
+ /* nothing */;
+ /* Find end of the path component */
+ for (end = start; *end && !is_dir_sep(*end); end++)
+ /* nothing */;
+
+ strbuf_add(next, start, end - start);
+ /* remove the component from 'remaining' */
+ strbuf_remove(remaining, 0, end - remaining->buf);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#define MAXSYMLINKS 5
/*
* Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)
* absolute_path().) The return value is a pointer to a static
* buffer.
*
- * The input and all intermediate paths must be shorter than MAX_PATH.
* The directory part of path (i.e., everything up to the last
* dir_sep) must denote a valid, existing directory, but the last
* component need not exist. If die_on_error is set, then die with an
@@ -33,22 +62,16 @@ int is_directory(const char *path)
*/
static const char *real_path_internal(const char *path, int die_on_error)
{
- static struct strbuf sb = STRBUF_INIT;
+ static struct strbuf resolved = STRBUF_INIT;
+ struct strbuf remaining = STRBUF_INIT;
+ struct strbuf next = STRBUF_INIT;
+ struct strbuf symlink = STRBUF_INIT;
char *retval = NULL;
-
- /*
- * If we have to temporarily chdir(), store the original CWD
- * here so that we can chdir() back to it at the end of the
- * function:
- */
- struct strbuf cwd = STRBUF_INIT;
-
- int depth = MAXDEPTH;
- char *last_elem = NULL;
+ int num_symlinks = 0;
struct stat st;
/* We've already done it */
- if (path == sb.buf)
+ if (path == resolved.buf)
return path;
if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&sb);
- strbuf_addstr(&sb, path);
-
- while (depth--) {
- if (!is_directory(sb.buf)) {
- char *last_slash = find_last_dir_sep(sb.buf);
- if (last_slash) {
- last_elem = xstrdup(last_slash + 1);
- strbuf_setlen(&sb, last_slash - sb.buf + 1);
- } else {
- last_elem = xmemdupz(sb.buf, sb.len);
- strbuf_reset(&sb);
- }
+ strbuf_reset(&resolved);
+
+ if (is_absolute_path(path)) {
+ /* absolute path; start with only root as being resolved */
+ strbuf_addch(&resolved, '/');
+ strbuf_addstr(&remaining, path + 1);
+ } else {
+ /* relative path; can use CWD as the initial resolved path */
+ if (strbuf_getcwd(&resolved)) {
+ if (die_on_error)
+ die_errno("Could not get current working directory");
+ else
+ goto error_out;
+ }
+ strbuf_addstr(&remaining, path);
+ }
+
+ /* Iterate over the remaining path components */
+ while (remaining.len > 0) {
+ get_next_component(&next, &remaining);
+
+ if (next.len == 0) {
+ continue; /* empty component */
+ } else if (next.len == 1 && !strcmp(next.buf, ".")) {
+ continue; /* '.' component */
+ } else if (next.len == 2 && !strcmp(next.buf, "..")) {
+ /* '..' component; strip the last path component */
+ strip_last_component(&resolved);
+ continue;
}
- if (sb.len) {
- if (!cwd.len && strbuf_getcwd(&cwd)) {
+ /* append the next component and resolve resultant path */
+ if (resolved.buf[resolved.len - 1] != '/')
+ strbuf_addch(&resolved, '/');
+ strbuf_addbuf(&resolved, &next);
+
+ if (lstat(resolved.buf, &st)) {
+ /* error out unless this was the last component */
+ if (!(errno == ENOENT && !remaining.len)) {
if (die_on_error)
- die_errno("Could not get current working directory");
+ die_errno("Invalid path '%s'",
+ resolved.buf);
else
goto error_out;
}
+ } else if (S_ISLNK(st.st_mode)) {
+ ssize_t len;
+ strbuf_reset(&symlink);
- if (chdir(sb.buf)) {
+ if (num_symlinks++ > MAXSYMLINKS) {
if (die_on_error)
- die_errno("Could not switch to '%s'",
- sb.buf);
+ die("More than %d nested symlinks "
+ "on path '%s'", MAXSYMLINKS, path);
else
goto error_out;
}
- }
- if (strbuf_getcwd(&sb)) {
- if (die_on_error)
- die_errno("Could not get current working directory");
- else
- goto error_out;
- }
- if (last_elem) {
- if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
- strbuf_addch(&sb, '/');
- strbuf_addstr(&sb, last_elem);
- free(last_elem);
- last_elem = NULL;
- }
-
- if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
- struct strbuf next_sb = STRBUF_INIT;
- ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
+ len = strbuf_readlink(&symlink, resolved.buf,
+ st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- sb.buf);
+ resolved.buf);
else
goto error_out;
}
- strbuf_swap(&sb, &next_sb);
- strbuf_release(&next_sb);
- } else
- break;
+
+ if (is_absolute_path(symlink.buf)) {
+ /*
+ * absolute symlink
+ * reset resolved path to root
+ */
+ strbuf_setlen(&resolved, 1);
+ } else {
+ /*
+ * relative symlink
+ * strip off the last component since it will
+ * be replaced with the contents of the symlink
+ */
+ strip_last_component(&resolved);
+ }
+
+ /*
+ * if there are still remaining components to resolve
+ * then append them to symlink
+ */
+ if (remaining.len) {
+ strbuf_addch(&symlink, '/');
+ strbuf_addbuf(&symlink, &remaining);
+ }
+
+ /*
+ * use the symlink as the remaining components that
+ * need to be resloved
+ */
+ strbuf_swap(&symlink, &remaining);
+ }
}
- retval = sb.buf;
+ retval = resolved.buf;
+
error_out:
- free(last_elem);
- if (cwd.len && chdir(cwd.buf))
- die_errno("Could not change back to '%s'", cwd.buf);
- strbuf_release(&cwd);
+ //strbuf_release(&resolved);
+ strbuf_release(&remaining);
+ strbuf_release(&next);
+ strbuf_release(&symlink);
return retval;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Brandon Williams @ 2016-12-05 19:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: David.Turner, git, sandals, hvoigt, gitster
In-Reply-To: <20161203003022.29797-16-sbeller@google.com>
On 12/02, Stefan Beller wrote:
>
> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
> ! test -s actual
> '
Should you use test_must_fail and not '!'?
--
Brandon Williams
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Brandon Williams @ 2016-12-05 19:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: David.Turner, git, sandals, hvoigt, gitster
In-Reply-To: <20161203003022.29797-18-sbeller@google.com>
On 12/02, Stefan Beller wrote:
> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
> + test_config checkout.recurseSubmodules 1 &&
> + git checkout base &&
> + git checkout -b advanced-base &&
> + git -C submodule commit --allow-empty -m "empty commit" &&
> + git add submodule &&
> + git commit -m "advance submodule" &&
> + git checkout base &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached base &&
> + git checkout advanced-base &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached advanced-base &&
> + git checkout --recurse-submodules base
> +'
> +
This test doesn't look like it looks into the submodule to see if the
submodule has indeed changed. Unless diff-index and diff-files recurse
into the submodules?
--
Brandon Williams
^ permalink raw reply
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Stefan Beller @ 2016-12-05 19:30 UTC (permalink / raw)
To: Brandon Williams
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <20161205192527.GA68588@google.com>
On Mon, Dec 5, 2016 at 11:25 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/02, Stefan Beller wrote:
>>
>> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
>> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
>> ! test -s actual
>> '
>
> Should you use test_must_fail and not '!'?
We use test_must_fail for git and '!' for non git thigns (test, grep etc),
as the test suite is about testing git.
The test_must_fail expects the command to be run to
* not reurn 0 (success)
* not segfault
* not return some other arbitrary return codes
indicating abnormal failure (125 IIRC)
So in a way test_must_fail translates to:
"I want to run this git command and it should fail
gracefully because at this state, it is the best git can do"
The '!' however is just inverting the boolean expression.
We assume test, grep, et al. to be flawless here. ;)
Stefan
^ permalink raw reply
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Brandon Williams @ 2016-12-05 19:31 UTC (permalink / raw)
To: Stefan Beller
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <CAGZ79kbEtR7_6ZvBsjkc=8q+nnq9FoPv9HNWdRyuR2CQGqQ2oA@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> On Mon, Dec 5, 2016 at 11:25 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/02, Stefan Beller wrote:
> >>
> >> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
> >> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
> >> ! test -s actual
> >> '
> >
> > Should you use test_must_fail and not '!'?
>
> We use test_must_fail for git and '!' for non git thigns (test, grep etc),
> as the test suite is about testing git.
>
> The test_must_fail expects the command to be run to
> * not reurn 0 (success)
> * not segfault
> * not return some other arbitrary return codes
> indicating abnormal failure (125 IIRC)
>
> So in a way test_must_fail translates to:
> "I want to run this git command and it should fail
> gracefully because at this state, it is the best git can do"
>
> The '!' however is just inverting the boolean expression.
> We assume test, grep, et al. to be flawless here. ;)
Ah, alright. Thanks for the info :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 19:57 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
On Mon, Dec 5, 2016 at 10:58 AM, Brandon Williams <bmwill@google.com> wrote:
> The current implementation of real_path uses chdir() in order to resolve
> symlinks. Unfortunately this isn't thread-safe as chdir() affects a
> process as a whole and not just an individual thread. Instead perform
> the symlink resolution by hand so that the calls to chdir() can be
> removed, making real_path reentrant.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
Thanks for working on this, some comments below:
>
> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> + if (path->len > 1) {
> + char *last_slash = find_last_dir_sep(path->buf);
What happens when there is no dir_sep?
> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
It's more than just getting it, it also chops it off of 'remaining' ?
> + strbuf_reset(&resolved);
> +
> + if (is_absolute_path(path)) {
> + /* absolute path; start with only root as being resolved */
> + strbuf_addch(&resolved, '/');
> + strbuf_addstr(&remaining, path + 1);
This is where we would wait for input of Windows savy people.
> + } else {
> + /* relative path; can use CWD as the initial resolved path */
> + if (strbuf_getcwd(&resolved)) {
> + if (die_on_error)
> + die_errno("Could not get current working directory");
I am looking at xgetcwd, which words it slightly differently.
if (strbuf_getcwd(&sb))
die_errno(_("unable to get current working directory"));
Not sure if aligning them would be a good idea?
Going by "git grep die_errno" as well as our Coding guidelines,
we don't want to see capitalized error messages.
> +
> + if (next.len == 0) {
> + continue; /* empty component */
which means we resolve over path//with//double//slashes just fine,
as well as /./ parts. :)
> }
>
> - if (sb.len) {
> - if (!cwd.len && strbuf_getcwd(&cwd)) {
> + /* append the next component and resolve resultant path */
"resultant" indicates you have a math background. :)
But I had to look it up, I guess it is fine that way,
though "resulting" may cause less mental friction
for non native speakers.
> + if (!(errno == ENOENT && !remaining.len)) {
> if (die_on_error)
> - die_errno("Could not get current working directory");
> + die_errno("Invalid path '%s'",
> + resolved.buf);
> else
> goto error_out;
> }
> + } else if (S_ISLNK(st.st_mode)) {
As far as I can tell, we could keep the symlink strbuf
at a smaller scope here? (I was surprised how many strbufs
are declared at the beginning of the function)
> + //strbuf_release(&resolved);
This is why the cover letter toned down expectations ?
(no // as comment, maybe remove that line?)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-05 20:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161201235856.GL54082@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 12/01, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > I started taking a look at your http redirect series (I really should
>> > have taking a look at it sooner) and I see exactly what you're talking
>> > about. We can easily move this logic into a function to make it easier
>> > to generate the two whitelists.
>>
>> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
>> expect that a future reroll would make things better.
>
> Yeah I expected we would see an issue since both these series collide in
> http.c
>
> I'm sending out another reroll of this series so that in Jeff's he can
> just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> option, which should make this test stop barfing.
I was hoping to eventually merge Peff's series to older maintenance
tracks. How bad would it be if we rebased the v8 of this series
together with Peff's series to say v2.9 (or even older if it does
not look too bad)?
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 20:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
> static const char *real_path_internal(const char *path, int die_on_error)
> {
> - static struct strbuf sb = STRBUF_INIT;
> + static struct strbuf resolved = STRBUF_INIT;
Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
> /* We've already done it */
> - if (path == sb.buf)
> + if (path == resolved.buf)
> return path;
I wonder how often we run into this case; are there some callers explicitly
relying on real_path_internal being cheap for repeated calls?
(Maybe run the test suite with this early return instrumented? Not sure how
to assess the impact of removing the cheap out return optimization)
The long tail (i.e. the actual functionality) should actually be
faster, I'd imagine
as we do less than with using chdir.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 20:16 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CAGZ79katWewdwU3ZDYDj-QZEeersx9XDPZuTdMJG_u_62YwMsg@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> > static const char *real_path_internal(const char *path, int die_on_error)
> > {
> > - static struct strbuf sb = STRBUF_INIT;
> > + static struct strbuf resolved = STRBUF_INIT;
>
> Also by having this static here, it is not quite thread safe, yet.
>
> By removing the static here we cannot do the early cheap check as:
>
> > /* We've already done it */
> > - if (path == sb.buf)
> > + if (path == resolved.buf)
> > return path;
>
> I wonder how often we run into this case; are there some callers explicitly
> relying on real_path_internal being cheap for repeated calls?
> (Maybe run the test suite with this early return instrumented? Not sure how
> to assess the impact of removing the cheap out return optimization)
>
> The long tail (i.e. the actual functionality) should actually be
> faster, I'd imagine
> as we do less than with using chdir.
Depends on how expensive the chdir calls were. And I'm working to get
rid of the static buffer. Just need have the callers own the memory
first.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 20:12 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CAGZ79kauPdE1uiFSvBALkNiwXbnV6d6xhwLdWNQwRir_8rTG6Q@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> > +/* removes the last path component from 'path' except if 'path' is root */
> > +static void strip_last_component(struct strbuf *path)
> > +{
> > + if (path->len > 1) {
> > + char *last_slash = find_last_dir_sep(path->buf);
>
> What happens when there is no dir_sep?
There should always be a dir_sep since that only gets run if the passed
in path at least contains root '/'
>
> > +/* gets the next component in 'remaining' and places it in 'next' */
> > +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> > +{
>
> It's more than just getting it, it also chops it off of 'remaining' ?
True, I can update the comment to reflect that.
> > + } else {
> > + /* relative path; can use CWD as the initial resolved path */
> > + if (strbuf_getcwd(&resolved)) {
> > + if (die_on_error)
> > + die_errno("Could not get current working directory");
>
> I am looking at xgetcwd, which words it slightly differently.
>
> if (strbuf_getcwd(&sb))
> die_errno(_("unable to get current working directory"));
>
> Not sure if aligning them would be a good idea?
>
> Going by "git grep die_errno" as well as our Coding guidelines,
> we don't want to see capitalized error messages.
K I can use the other msg.
> >
> > - if (sb.len) {
> > - if (!cwd.len && strbuf_getcwd(&cwd)) {
> > + /* append the next component and resolve resultant path */
>
> "resultant" indicates you have a math background. :)
> But I had to look it up, I guess it is fine that way,
> though "resulting" may cause less mental friction
> for non native speakers.
>
>
> > + if (!(errno == ENOENT && !remaining.len)) {
> > if (die_on_error)
> > - die_errno("Could not get current working directory");
> > + die_errno("Invalid path '%s'",
> > + resolved.buf);
> > else
> > goto error_out;
> > }
> > + } else if (S_ISLNK(st.st_mode)) {
>
> As far as I can tell, we could keep the symlink strbuf
> at a smaller scope here? (I was surprised how many strbufs
> are declared at the beginning of the function)
Yeah I can push it down in scope. There will be a bit more allocation
churn with the smaller scope but multiple symlinks should be rare?
Alternatively the 'next' buffer can be reused...I decided against that
initially due to readability. And yes, lots of string manipulation
requires lots of strbufs :)
> > + //strbuf_release(&resolved);
>
> This is why the cover letter toned down expectations ?
> (no // as comment, maybe remove that line?)
yep. It will be added back in though once the callers to real_path take
ownership of the memory.
--
Brandon Williams
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 20:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <xmqq7f7e5jsy.fsf@gitster.mtv.corp.google.com>
On 05/12/16 18:10, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Heh, that was the first version of the patch. However, I got to thinking
>> about why --abbrev=7 was there in the first place; the only reason I
>> could think of was to defeat local configuration to get a measure of
>> reproducibility.
>>
>> Unfortunately, you can't get the 'auto' behaviour from --abbrev
>> (on the pu branch):
>>
>> $ ./git describe --abbrev=-1
>> v2.11.0-286-g109e8
>> $ ./git describe --abbrev=0
>> v2.11.0
>> $ ./git describe
>> v2.11.0-286-g109e8a99d
>> $
>
> What is the reason why the last one is undesirable? Is it because
> the user may have core.abbrev set to some value in the configuration
> and you want to override it to force "auto"?
As I said, the original version of the patch just removed the
--abbrev=7, but then I started to think about why you might have
used --abbrev in the first place (first in commit 9b88fcef7 and
again in commit bf505158d). Making sure to override the configuration
was the only thing I could come up with. So, I was hoping you could
remember why! :-P
(I assumed it was to force a measure of uniformity/reproducibility).
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v4 1/3] update-unicode.sh: automatically download newer definition files
From: Junio C Hamano @ 2016-12-05 20:31 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Beat Bolli, git
In-Reply-To: <20161204075800.GA2415@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
> On Sat, Dec 03, 2016 at 10:00:47PM +0100, Beat Bolli wrote:
>> Checking just for the unicode data files' existence is not sufficient;
>> we should also download them if a newer version exists on the Unicode
>> consortium's servers. Option -N of wget does this nicely for us.
>>
>> Reviewed-by: Torsten Boegershausen <tboegi@web.de>
>
> Minor remark (Not sure if this motivates v5, may be Junio can fix it locally?)
> s/oe/ö/
>
> Beside this: Thanks again (and I learned about the -N option of wget)
Will fix up while queuing (only 1/3 needs it, 2/3 has it right).
Also, I'll do s/update-unicode.sh/update_unicode.sh/ on the title
and the message to match the reality. At some point we might want
to fix the reality to match people's expectations, though.
Thanks.
^ permalink raw reply
* git repo vs project level authorization
From: ken edward @ 2016-12-05 20:33 UTC (permalink / raw)
To: git
I am currently using svn with apache+mod_dav_svn to have a single
repository with multiple projects. Each of the projects is controlled
by an access control file that lists the project path and the allowed
usernames.
Does git have this also? where is the doc?
Ken
^ permalink raw reply
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Junio C Hamano @ 2016-12-05 20:36 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Krey, git
In-Reply-To: <20161203162318.uv27n4uhylobegto@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
>
>> > OK. I'm not sure why you would want to create an empty commit in such a
>> > case.
>>
>> User: Ok tool, make me a pullreq.
>>
>> Tool: But you haven't mentioned any issue
>> in your commit messages. Which are they?
>>
>> User: Ok, that would be A-123.
>>
>> Tool: git commit --allow-empty -m 'FIX: A-123'
>
> OK. I think "tool" is slightly funny here, but I get that is part of the
> real world works. Thanks for illustrating.
I am not sure if I understand. Why isn't the FIX: thing added to
the commit being pulled by amending it? Would the convention be for
the responder of a pull-request to fetch and drop the tip commit?
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 20:38 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <20161205201237.GD68588@google.com>
On Mon, Dec 5, 2016 at 12:12 PM, Brandon Williams <bmwill@google.com> wrote:
>> > + if (path->len > 1) {
>> > + char *last_slash = find_last_dir_sep(path->buf);
>>
>> What happens when there is no dir_sep?
>
> There should always be a dir_sep since that only gets run if the passed
> in path at least contains root '/'
Oh, sure, that makes sense. When porting/running this on Windows, does
the assumption still hold?
>> if (strbuf_getcwd(&sb))
>> die_errno(_("unable to get current working directory"));
>>
>> Not sure if aligning them would be a good idea?
>>
>> Going by "git grep die_errno" as well as our Coding guidelines,
>> we don't want to see capitalized error messages.
>
> K I can use the other msg.
Well this wasn't a rhetorical question, but I was genuine wondering
if that was worth it.
When having different error messages in different places,
it makes debugging easier, because you have fewer starting points.
But this function is deep down in the stack, such that you would expect
other error messages to also show up , so I dunno.
>> > + } else if (S_ISLNK(st.st_mode)) {
>>
>> As far as I can tell, we could keep the symlink strbuf
>> at a smaller scope here? (I was surprised how many strbufs
>> are declared at the beginning of the function)
>
> Yeah I can push it down in scope. There will be a bit more allocation
> churn with the smaller scope but multiple symlinks should be rare?
> Alternatively the 'next' buffer can be reused...I decided against that
> initially due to readability.
I'd second to not reuse 'next'. :)
I guess we could keep the less churn-y version then.
> And yes, lots of string manipulation
> requires lots of strbufs :)
^ permalink raw reply
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Junio C Hamano @ 2016-12-05 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Krey, git
In-Reply-To: <20161203043254.7ozjyucfn6uivnsh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:
>
>> --only is implied when paths are present, and required
>> them unless --amend. But with --allow-empty it should
>> be allowed as well - it is the only way to create an
>> empty commit in the presence of staged changes.
>
> OK. I'm not sure why you would want to create an empty commit in such a
> case. But I do agree that this seems like a natural outcome for "--only
> --allow-empty". So whether it is particularly useful or not, it seems
> like the right thing to do. The patch itself looks good to me.
Slightly related topic.
>> - if (argc == 0 && (also || (only && !amend)))
>> + if (argc == 0 && (also || (only && !amend && !allow_empty)))
>> die(_("No paths with --include/--only does not make sense."));
>> if (argc == 0 && only && amend)
>> only_include_assumed = _("Clever... amending the last one with dirty index.");
>
We allow "-o --amend" without no pathspec because that is how you
would reword without changing the tree object in the tip commit, and
we reward the user who figured out such an esoteric use with a
message "Clever...". I do not think if people who say "I want to
create an empty commit but I already have added changes to the
index" deserve the same "Clever..." praise, so I will not suggest
adding another message above.
More seriously, I suspect that the message outlived its usefulness.
If we wanted to make the "use --amend -o without pathspec if you
want to reword the tip one without touching its tree" easier to
discover, the place to do so is in the documentation, not a message
that is given as a reward to those who already discovered it.
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 20:53 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Vinicius Kursancew, larsxschneider
In-Reply-To: <20161202224319.5385-2-luke@diamand.org>
Luke Diamand <luke@diamand.org> writes:
> Teach git-p4 about git-workspaces.
Is this what we call "git worktree", or something else?
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> git-p4.py | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0c4f2afd2..5e2db1919 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -566,6 +566,12 @@ def isValidGitDir(path):
> if (os.path.exists(path + "/HEAD")
> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> return True;
> +
> + # git workspace directory?
> + if (os.path.exists(path + "/HEAD")
> + and os.path.exists(path + "/gitdir")):
> + return True
> +
> return False
>
> def parseRevision(ref):
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Luke Diamand @ 2016-12-05 21:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq8tru3xom.fsf@gitster.mtv.corp.google.com>
On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> Teach git-p4 about git-workspaces.
>
> Is this what we call "git worktree", or something else?
Ah, I think you're right!
>
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>> git-p4.py | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 0c4f2afd2..5e2db1919 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -566,6 +566,12 @@ def isValidGitDir(path):
>> if (os.path.exists(path + "/HEAD")
>> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
>> return True;
>> +
>> + # git workspace directory?
>> + if (os.path.exists(path + "/HEAD")
>> + and os.path.exists(path + "/gitdir")):
>> + return True
>> +
>> return False
>>
>> def parseRevision(ref):
^ permalink raw reply
* Re: [PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default
From: Ori Rawlings @ 2016-12-05 21:19 UTC (permalink / raw)
To: Git Users
In-Reply-To: <20161204140311.26269-1-larsxschneider@gmail.com>
Looks good to me, too.
-r flag seems to be supported as far back as I can search in the Helix
release notes.
Ori Rawlings
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 21:24 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <CAE5ih78Y_AbfgtW_6zMKLC8NzBxCKSagrgrjtfWZVOEwaAg6ZA@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> Teach git-p4 about git-workspaces.
>>
>> Is this what we call "git worktree", or something else?
>
> Ah, I think you're right!
Then I'll queue it like the attached.
HOWEVER.
How fast does isValidGitDir() function need to be? The primary one
seems to check HEAD (but it does not notice a non-repository that
has a directory with that name), refs and objects (but it does not
notice a non-repository that has a non-directory with these names),
and this new one uses a test that is even more sloppy.
What I am trying to get at is if we want to use a single command
that can be given a path and answer "Yes, that is a repository"
here, and that single command should know how the repository should
look like. I offhand do not know we already have such a command we
can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
perhaps we would want one, so that not just "git p4" but other
scripted Porcelains can make use of it?
-- >8 --
From: Luke Diamand <luke@diamand.org>
Date: Fri, 2 Dec 2016 22:43:18 +0000
Subject: [PATCH] git-p4: support secondary working trees managed by "git worktree"
Teach git-p4 about them.
Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-p4.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..b3c50ae7e5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
if (os.path.exists(path + "/HEAD")
and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
return True;
+
+ # secondary working tree managed by "git worktree"?
+ if (os.path.exists(path + "/HEAD")
+ and os.path.exists(path + "/gitdir")):
+ return True
+
return False
def parseRevision(ref):
--
2.11.0-222-g22b1346184
^ permalink raw reply related
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