* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Junio C Hamano @ 2016-10-26 23:59 UTC (permalink / raw)
To: Stefan Beller; +Cc: Matt McCutchen, git
In-Reply-To: <CAGZ79kaw0s_PC2AstRVwFT8N1CJVC_7yQfC19zPzRjAqkSpMDg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> - We have to make separate commits and manage corresponding topic
>> branches for the superproject and subprojects.
>
> Well yeah, that is how submodule work on a conceptual level.
> While having multiple commits may seem like overhead, note
> the subtle difference for these commits. One if deep down in the
> stack patching one of the submodules, the other is a high level
> commit advancing the submodule pointer.
>
> Note that the target audience of these two commit messages
> might be vastly different, hence can be worded differently.
> (The submodule describing how you fixed e.g. a memleak or race condition
> and the superproject describes on why you needed to include that submodule,
> e.g. because you switched your toplevel application to use threads.)
Both good points.
Another thing to keep in mind is that in a well-organized project,
it is expected that you would have multiple commits in a submodule,
solving one single issue that is needed by the superproject in a
finer grained way, before the resulting submodule tip is recorded in
the tree of the superproject in one commit. IOW, between the time
the superproject's history moves by one commit, the submodule may
have multiple commits in order for the submodule to become ready to
be consumed by the superproject.
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-27 0:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <xmqqoa26aek6.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -53,19 +57,32 @@ value of the attribute for the path.
>> Querying Specific Attributes
>> ----------------------------
>>
>> -* Prepare `struct git_attr_check` using git_attr_check_initl()
>> +* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
>> function, enumerating the names of attributes whose values you are
>> interested in, terminated with a NULL pointer. Alternatively, an
>> - empty `struct git_attr_check` can be prepared by calling
>> - `git_attr_check_alloc()` function and then attributes you want to
>> - ask about can be added to it with `git_attr_check_append()`
>> - function.
>> -
>> -* Call `git_check_attr()` to check the attributes for the path.
>> -
>> -* Inspect `git_attr_check` structure to see how each of the
>> - attribute in the array is defined for the path.
>> -
>> + empty `struct git_attr_check` as allocated by git_attr_check_alloc()
>> + can be prepared by calling `git_attr_check_alloc()` function and
>> + then attributes you want to ask about can be added to it with
>> + `git_attr_check_append()` function.
>
> I think that my version that was discarded forbade appending once
> you started to use the check for querying, because the check was
> meant to be used as a key for an attr-stack and the check-specific
> attr-stack was planned to keep only the attrs the check is
> interested in (and appending is to change the set of attrs the check
> is interested in, invalidating the attr-stack at that point).
>
> If you lost that restriction, that is good (I didn't check the
> implementation, though). Otherwise we'd need to say something here.
That restriction still applies. Though I think mentioning it in the
paragraph where we describe querying makes more sense.
> initialization?
done
>
> Grammo? "either on the stack, or dynamically in the heap"?
done
>
> Having result defined statically is not thread safe for that
> reason. It is not clear what you mean by "The call to initialize
> the result"; having it on the stack or have one dynamically on the
> heap ought to be thread safe.
done
>> -}
>> + static struct git_attr_check *check;
>> + git_attr_check_initl(check, "crlf", "ident", NULL);
>
> I think you are still missing "&" here.
done
>> + if (check)
>> + return; /* already done */
>> check = git_attr_check_alloc();
>
> You may want to say that this is thread-unsafe.
It is not; see the implementation:
void git_attr_check_append(struct git_attr_check *check,
const struct git_attr *attr)
{
int i;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
attr_lock();
for (i = 0; i < check->check_nr; i++)
if (check->attr[i] == attr)
break;
if (i == check->check_nr) {
ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
check->attr[check->check_nr++] = attr;
}
attr_unlock();
}
>> +* Call `git_all_attrs()`.
>
> Hmph, the caller does not know what attribute it is interested in,
> and it is unclear "how" the former needs to be set up from this
> description. Should it prepare an empty one that can be appended?
>
Good point on clarifying this one. It is just needed to have
NULL pointers around:
struct git_attr_check *check = NULL;
struct git_attr_result *result = NULL;
git_all_attrs(full_path, &local_check, &result);
// proceed as in the case above
All comments from above yield the following diff:
diff --git a/Documentation/technical/api-gitattributes.txt
b/Documentation/technical/api-gitattributes.txt
index 4d8ef93..d221736 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -67,19 +67,21 @@ Querying Specific Attributes
Both ways with `git_attr_check_initl()` as well as the
alloc and append route are thread safe, i.e. you can call it
from different threads at the same time; when check determines
- the initialzisation is still needed, the threads will use a
+ the initialization is still needed, the threads will use a
single global mutex to perform the initialization just once, the
others will wait on the the thread to actually perform the
initialization.
-* Allocate an array of `struct git_attr_result` either statically on the
- as a variable on the stack or dynamically via `git_attr_result_alloc`
- when the result size is not known at compile time. The call to initialize
+* Allocate an array of `struct git_attr_result` either on the stack
+ or via `git_attr_result_alloc` on the heap when the result size
+ is not known at compile time. The call to initialize
the result is not thread safe, because different threads need their
own thread local result anyway.
* Call `git_check_attr()` to check the attributes for the path,
the given `git_attr_result` will be filled with the result.
+ You must not change the `struct git_attr_check` after calling
+ `git_check_attr()`.
* Inspect each `git_attr_result` structure to see how
each of the attribute in the array is defined for the path.
@@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are set
for different paths.
const char *path;
struct git_attr_result result[2];
- git_check_attr(path, check, result);
+ git_check_attr(path, &check, result);
------------
. Act on `result.value[]`:
@@ -155,10 +157,20 @@ To get the values of all attributes associated
with a file:
* Setup a local variables for the question
`struct git_attr_check` as well as a pointer where the result
- `struct git_attr_result` will be stored.
+ `struct git_attr_result` will be stored. Both should be initialized
+ to NULL.
+
+------------
+ struct git_attr_check *check = NULL;
+ struct git_attr_result *result = NULL;
+------------
* Call `git_all_attrs()`.
+------------
+ git_all_attrs(full_path, &check, &result);
+------------
+
* Iterate over the `git_attr_check.attr[]` array to examine the
attribute names. The name of the attribute described by a
`git_attr_check.attr[]` object can be retrieved via
^ permalink raw reply related
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-27 0:16 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <CAGZ79kaR4DddoHQNUUvRAY=_PK5qqS=ws_Wkfa-EXT2seN5b=A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> +* Allocate an array of `struct git_attr_result` either on the stack
> + or via `git_attr_result_alloc` on the heap when the result size
> + is not known at compile time. The call to initialize
> the result is not thread safe, because different threads need their
> own thread local result anyway.
Do you want to keep the last sentence? "The call to initialize the
result is not thread safe..."? Is that true?
> @@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are set
> for different paths.
> const char *path;
> struct git_attr_result result[2];
>
> - git_check_attr(path, check, result);
> + git_check_attr(path, &check, result);
What's the point of this change? Isn't check typically a pointer
already?
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-27 0:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <xmqqfuniabo6.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 5:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +* Allocate an array of `struct git_attr_result` either on the stack
>> + or via `git_attr_result_alloc` on the heap when the result size
>> + is not known at compile time. The call to initialize
>> the result is not thread safe, because different threads need their
>> own thread local result anyway.
>
> Do you want to keep the last sentence? "The call to initialize the
> result is not thread safe..."? Is that true?
I'll drop that sentence, as it overstates the situation.
To explain, you can either have:
struct git_attr_result result[2];
or
struct git_attr_result *result = git_attr_result_alloc(check);
and both are running just fine in a thread. However you should not
make that variable static. But maybe that is too much common sense
and hence confusing.
>
>> @@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are set
>> for different paths.
>> const char *path;
>> struct git_attr_result result[2];
>>
>> - git_check_attr(path, check, result);
>> + git_check_attr(path, &check, result);
>
> What's the point of this change? Isn't check typically a pointer
> already?
This ought to go to
git_attr_check_initl(&check, "crlf", "ident", NULL);
instead.
>
^ permalink raw reply
* Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 0:22 UTC (permalink / raw)
To: git
I want a feature. It may be a bad-idea(tm). Advice appreciated.
I want git to be able to include, in its gitignore files, sub-files of
ignores or have it understand a directory of ignore files. Or both.
The use case for this is where I did not write my own rules, but I want
to keep them updated. https://github.com/github/gitignore is a damn good
resource, but I want to pull it and include relevant bits project by
project and/or system wide. I don't want to have to update many projects
manually if that, or any other, repo changes.
A very brief look at dir.c would indicate that a recursive call from
add_excludes to itself when it parses some sort of include tag would do
it within a file. I'm sure it'd be pretty straight forward to hook into
something in dir.c to parse directories too.
I'm thinking something like ". path/to/include/file" in an ignore file,
and/or creating .gitignore.d and/or allowing $HOME/.config/git/ignore
and $GIT_DIR/info/exclude to be directories. Or some sane and consistent
mixture of these things.
In the case of a directory the plan would be to add links to files
stored/sourced elsewhere. This does pose a precedence question which I
haven't thought about yet, but probably makes it too hard for the
limited value it brings.
There is also the issue of malicious/accidental recursion which I
haven't thought about deeply either.
I would like to know the desirability/practicality/stupidity of such a
feature as I believe it is within my skillset to implement it.
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-27 0:49 UTC (permalink / raw)
To: Stefan Beller; +Cc: pclouds, git, Johannes.Schindelin, j6t, peff, bmwill, simon
In-Reply-To: <20161026224104.31844-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> extern struct git_attr *git_attr(const char *);
> ...
> +extern void git_attr_check_append(struct git_attr_check *,
> + const struct git_attr *);
Another thing. Do we still need to expose git_attr() to the outside
callers? If we change git_attr_check_append() to take "const char *"
as its second parameter, can we retire it from the public interface?
It being an "intern" function, by definition it is not thread-safe.
Its use from prepare_attr_stack() inside git_check_attr() that takes
the Big Attribute Subsystem Lock is safe, but the callers that do
struct git_attr_check *check = ...;
struct git_attr *text_attr = git_attr("text");
git_attr_check_append(check, text_attr);
would risk racing to register the same entry to the "all the
attributes in the known universe" table.
If the attribute API does not have to expose git_attr(const char *)
to the external callers at all, that would be ideal. Otherwise, we
would need to rename the current git_attr() that is used internally
under the Big Lock to
static struct git_attr *git_attr_locked(const char*)
that is defined inside attr.c, and then provide the external version
as a thin wrapper that calls it under the Big Lock.
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-27 0:50 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <CAGZ79kY00mtzwL4kSz6oXSAb-39=axeTP-ax_FxWcL7Z-sAA4w@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> To explain, you can either have:
> struct git_attr_result result[2];
> or
> struct git_attr_result *result = git_attr_result_alloc(check);
> and both are running just fine in a thread. However you should not
> make that variable static. But maybe that is too much common sense
> and hence confusing.
Yup, if you spelled it out that does make sense, but the description
given in the patch was just misleading.
Thanks.
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Matt McCutchen @ 2016-10-27 1:52 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <CAGZ79kaw0s_PC2AstRVwFT8N1CJVC_7yQfC19zPzRjAqkSpMDg@mail.gmail.com>
Hi Stefan,
I appreciate the effort to remove obstacles to the use of submodules!
It looks like a custom tool is probably still our best option at this
time, though we can always switch back to submodules later.
On Wed, 2016-10-26 at 16:23 -0700, Stefan Beller wrote:
> On Wed, Oct 26, 2016 at 4:07 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:
> > - We have to make separate commits and manage corresponding topic
> > branches for the superproject and subprojects.
>
> Well yeah, that is how submodule work on a conceptual level.
> While having multiple commits may seem like overhead, note
> the subtle difference for these commits. One if deep down in the
> stack patching one of the submodules, the other is a high level
> commit advancing the submodule pointer.
>
> Note that the target audience of these two commit messages
> might be vastly different, hence can be worded differently.
> (The submodule describing how you fixed e.g. a memleak or race condition
> and the superproject describes on why you needed to include that submodule,
> e.g. because you switched your toplevel application to use threads.)
I understand one can adhere to that philosophy, but I don't see the
practical benefit of doing so in our case compared to using a "git
subtree"-like approach and making a single commit. It would just be us
looking at both commits. If we do upstream any of the library changes,
we'll probably have to rework them anyway.
> > - A diff of the superproject doesn't include the content of
> > subprojects.
> Although this is just Git, you probably also have a code review system that
> would need that change as well.
Indeed. We currently use Bitbucket. I'd be open to switching, though
maybe not just for this.
> Is there anything else besides these 3 points that encourages you to
> switch away from submodules?
Those 3 are the ongoing pain points I can think of. There are a few
other drawbacks compared to "git subtree" that come up less often:
1b. On another project, I was working with a teammate who was new to
version control and not very careful, who forgot to run "git submodule
update" and ended up committing back the old submodule pointer.
Thankfully, this hasn't happened yet on my current project.
4. I pushed several dangling submodule pointers before I learned I
could set push.recurseSubmodules = check. This isn't the default; each
developer has to do it manually. (In theory, I could put such things
in a setup script for them to run if they trust me.)
5. Stashing changes to both the superproject and the subproject takes
more steps.
6. I use multiple worktrees (with "git worktree") because of #5 and
also so that I can run two versions of the application at the same time
and compare the behavior. Using "git worktree" with submodules is
officially unsupported, though I was able to get things working by
manually editing some files.
7. We have to set up a repository for each subproject on our hosting
service. Anyone who forks our application and modifies a subproject
has to set up a subproject repository and carry a change to .gitmodules
to point to their repository. If we use relative URLs in .gitmodules,
it's even worse: anyone who forks our application has to set up
repositories for all the subprojects, even if they don't modify them.
Matt
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Stefan Beller @ 2016-10-27 2:03 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1477533150.2764.147.camel@mattmccutchen.net>
On Wed, Oct 26, 2016 at 6:52 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:
> Hi Stefan,
>
> I appreciate the effort to remove obstacles to the use of submodules!
> It looks like a custom tool is probably still our best option at this
> time, though we can always switch back to submodules later.
>
> On Wed, 2016-10-26 at 16:23 -0700, Stefan Beller wrote:
>> On Wed, Oct 26, 2016 at 4:07 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:
>> > - We have to make separate commits and manage corresponding topic
>> > branches for the superproject and subprojects.
>>
>> Well yeah, that is how submodule work on a conceptual level.
>> While having multiple commits may seem like overhead, note
>> the subtle difference for these commits. One if deep down in the
>> stack patching one of the submodules, the other is a high level
>> commit advancing the submodule pointer.
>>
>> Note that the target audience of these two commit messages
>> might be vastly different, hence can be worded differently.
>> (The submodule describing how you fixed e.g. a memleak or race condition
>> and the superproject describes on why you needed to include that submodule,
>> e.g. because you switched your toplevel application to use threads.)
>
> I understand one can adhere to that philosophy, but I don't see the
> practical benefit of doing so in our case compared to using a "git
> subtree"-like approach and making a single commit. It would just be us
> looking at both commits. If we do upstream any of the library changes,
> we'll probably have to rework them anyway.
>
>> > - A diff of the superproject doesn't include the content of
>> > subprojects.
>
>> Although this is just Git, you probably also have a code review system that
>> would need that change as well.
>
> Indeed. We currently use Bitbucket. I'd be open to switching, though
> maybe not just for this.
>
>> Is there anything else besides these 3 points that encourages you to
>> switch away from submodules?
>
> Those 3 are the ongoing pain points I can think of. There are a few
> other drawbacks compared to "git subtree" that come up less often:
>
> 1b. On another project, I was working with a teammate who was new to
> version control and not very careful, who forgot to run "git submodule
> update" and ended up committing back the old submodule pointer.
> Thankfully, this hasn't happened yet on my current project.
>
> 4. I pushed several dangling submodule pointers before I learned I
> could set push.recurseSubmodules = check. This isn't the default; each
> developer has to do it manually. (In theory, I could put such things
> in a setup script for them to run if they trust me.)
There is a current series in flight/for review that makes "check" default.
(It is blocked as check has some performance issues when having lots
of commits to be pushed, so it may take a while and not show up in the
next release)
>
> 5. Stashing changes to both the superproject and the subproject takes
> more steps.
True, so you'd want to have a `git stash --recurse-submodules={yes,no}`
where the command line option is configurable, so you don't have to type
it all the time?
>
> 6. I use multiple worktrees (with "git worktree") because of #5 and
> also so that I can run two versions of the application at the same time
> and compare the behavior. Using "git worktree" with submodules is
> officially unsupported, though I was able to get things working by
> manually editing some files.
Heh, true. I made an attempt on fixing git worktree a few weeks ago, but that
did not go anywhere, but it's still on the TODO list.
You can use git clone --reference with --recurse-submodule though when
having bandwidth concerns. It doesn't deliver the full worktree experience
though as it would be separate clones with shared object store.
>
> 7. We have to set up a repository for each subproject on our hosting
> service. Anyone who forks our application and modifies a subproject
> has to set up a subproject repository and carry a change to .gitmodules
> to point to their repository. If we use relative URLs in .gitmodules,
> it's even worse: anyone who forks our application has to set up
> repositories for all the subprojects, even if they don't modify them.
>
> Matt
Yeah the model of referencing in gitmodules is conceptually broken.
I don't even claim it is on my todo list. ;)
Thanks for pointing out the issues though. they align to what
we plan on doing for submodules, so ... the plan actually makes
sense :)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-27 2:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <xmqqbmy6aa6b.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> extern struct git_attr *git_attr(const char *);
>> ...
>> +extern void git_attr_check_append(struct git_attr_check *,
>> + const struct git_attr *);
>
> Another thing. Do we still need to expose git_attr() to the outside
> callers? If we change git_attr_check_append() to take "const char *"
> as its second parameter, can we retire it from the public interface?
>
> It being an "intern" function, by definition it is not thread-safe.
> Its use from prepare_attr_stack() inside git_check_attr() that takes
> the Big Attribute Subsystem Lock is safe, but the callers that do
>
> struct git_attr_check *check = ...;
> struct git_attr *text_attr = git_attr("text");
>
> git_attr_check_append(check, text_attr);
>
> would risk racing to register the same entry to the "all the
> attributes in the known universe" table.
>
> If the attribute API does not have to expose git_attr(const char *)
> to the external callers at all, that would be ideal. Otherwise, we
> would need to rename the current git_attr() that is used internally
> under the Big Lock to
>
> static struct git_attr *git_attr_locked(const char*)
>
> that is defined inside attr.c, and then provide the external version
> as a thin wrapper that calls it under the Big Lock.
>
>
Yeah, I can make it work without exposing struct git_attr.
However then the struct git_attr_check will contain more of
internals exposed. (As the header file did not know the size
of git_attr, the patch under discussion even must use a double pointer
to a git_attr inside the git_attr_check as the git_attr size is unknown)
So I'll look into removing that struct git_attr completely.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Stefan Beller @ 2016-10-27 2:22 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git@vger.kernel.org
In-Reply-To: <80919456-7563-2c16-ba23-ce4fcc2777de@pelly.co>
> The use case for this is where I did not write my own rules, but I want
> to keep them updated. https://github.com/github/gitignore is a damn good
> resource, but I want to pull it and include relevant bits project by
> project and/or system wide. I don't want to have to update many projects
> manually if that, or any other, repo changes.
.git/info/exclude could be a (sym)link to an up to date version
of the gitignore repo as a hack?
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Matt McCutchen @ 2016-10-27 2:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <CAGZ79kb1bb5e5hKpnkFqLOsPow5xt8zczWmZNxMMt5nA84tf-w@mail.gmail.com>
On Wed, 2016-10-26 at 19:03 -0700, Stefan Beller wrote:
> On Wed, Oct 26, 2016 at 6:52 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:
> > 4. I pushed several dangling submodule pointers before I learned I
> > could set push.recurseSubmodules = check. This isn't the default; each
> > developer has to do it manually. (In theory, I could put such things
> > in a setup script for them to run if they trust me.)
>
> There is a current series in flight/for review that makes "check" default.
> (It is blocked as check has some performance issues when having lots
> of commits to be pushed, so it may take a while and not show up in the
> next release)
Great! One other thing: IIRC, "check" does not distinguish between
different remotes. For example, suppose I fork a project that already
has a submodule and I have a pair of repositories that pull from the
"upstream" repositories and push to "origin" repositories for my
project. Suppose I upgrade to a new upstream version and find that I'm
(temporarily) able to use the upstream submodule without modifications.
The "check" feature won't stop me from pushing a pointer into the
"origin" superproject that points to a commit that exists in the
"upstream" subproject but not the "origin" subproject.
> > 5. Stashing changes to both the superproject and the subproject takes
> > more steps.
>
> True, so you'd want to have a `git stash --recurse-submodules={yes,no}`
> where the command line option is configurable, so you don't have to type
> it all the time?
Sounds good. I'm sure you realize this is not just a matter of running
"git stash" in each submodule because there are many ways the stash
stacks could get out of lockstep. The submodule content needs to be
incorporated into the superproject stash.
> Thanks for pointing out the issues though. they align to what
> we plan on doing for submodules, so ... the plan actually makes
> sense :)
Again, I'm thrilled you're working on this, even if I don't use it on
my current project.
Matt
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-27 4:13 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <CAGZ79kYiVxJwtAYsgks8LVNWvawWxdy=8Xpdrki-C_u15C3Z9w@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Yeah, I can make it work without exposing struct git_attr.
You completely misunderstood me. "struct git_attr" MUST be visible
to the users so that they can ask for the name in git_check.attr[0].
What would be nice to hide if you can is the function to intern a
string into a pointer to struct git_attr, i.e. git_attr() function.
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Peter Williams @ 2016-10-27 4:23 UTC (permalink / raw)
To: Junio C Hamano, Stefan Beller; +Cc: Matt McCutchen, git
In-Reply-To: <xmqqk2cuach3.fsf@gitster.mtv.corp.google.com>
On 27/10/16 09:59, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> - We have to make separate commits and manage corresponding topic
>>> branches for the superproject and subprojects.
>>
>> Well yeah, that is how submodule work on a conceptual level.
>> While having multiple commits may seem like overhead, note
>> the subtle difference for these commits. One if deep down in the
>> stack patching one of the submodules, the other is a high level
>> commit advancing the submodule pointer.
>>
>> Note that the target audience of these two commit messages
>> might be vastly different, hence can be worded differently.
>> (The submodule describing how you fixed e.g. a memleak or race condition
>> and the superproject describes on why you needed to include that submodule,
>> e.g. because you switched your toplevel application to use threads.)
>
> Both good points.
>
> Another thing to keep in mind is that in a well-organized project,
> it is expected that you would have multiple commits in a submodule,
> solving one single issue that is needed by the superproject in a
> finer grained way, before the resulting submodule tip is recorded in
> the tree of the superproject in one commit. IOW, between the time
> the superproject's history moves by one commit, the submodule may
> have multiple commits in order for the submodule to become ready to
> be consumed by the superproject.
>
>
I'm a relatively new user of submodules and I quite like them (having
tried a few other strategies for sharing common code between multiple
projects and found them quite painful) and find them fairly easy to use.
I especially like the fact that the submodule command isn't very
complicated and that the best method for managing commits, etc in the
submodule is to cd into their root directory and then treat them like
any other git repository (greatly reducing the amount of new stuff that
you have to learn in order to use them). Also, from my experience so
far, I see three different types of work going on within my workspaces
that include submodules:
1. I'm working on changes to the submodule and using the superproject
that it's checked out in to test those changes in which case most of the
change is occurring in the submodule with changes in the superproject
usually being small one related to API changes in the submodule.
2. I'm working on changes in the superproject and the only changes that
get made in the submodules are to fix bugs uncovered by the work in the
superproject.
3. I'm modifying a superproject to accommodate changes to a submodule
that's changed as a result of having changes pulled from another repository.
In none of these cases do I feel the desire/need to commit the changes
to the superproject and submodule(s) with a single commit command which
more or less agrees with your points.
However, for git commands such as diff/status whose job is to display
information it would be nice if they had a --recursive option to
override the default submodule diff/status and show details of the
changes in the submodules. Sometimes you want to see the big picture in
detail.
^ permalink raw reply
* Re: [PATCHv2 1/2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-27 5:44 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, git@vger.kernel.org, Johannes Schindelin,
Johannes Sixt, Jeff King, Brandon Williams, Simon Ruderich
In-Reply-To: <xmqq37jia0p8.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> Yeah, I can make it work without exposing struct git_attr.
>
> You completely misunderstood me. "struct git_attr" MUST be visible
> to the users so that they can ask for the name in git_check.attr[0].
>
> What would be nice to hide if you can is the function to intern a
> string into a pointer to struct git_attr, i.e. git_attr() function.
Even though that was not the primary point of my suggestion, I
actually think it is OK to make "struct git_attr" a structure that
is opaque to the users of the API if you wanted to. The attr_check
structure will have an array of pointers to "struct git_attr", and
the structure definition may be visible to the public attr.h header,
but the API users won't have to be able to dereference the pointer
"struct git_attr *". git_check.attr[0] would be a pointer to an
opaque structure from API users' point of view, that can be passed
to API function git_attr_name() to read its string.
What is nice to hide is the constructor of the structure. What it,
i.e. "struct git_attr *git_attr(const char *)", needs to do is to
(1) see if the attribute object with the same name already exists in
the table of "all known attributes in the universe", and if there
is, return that instance, (2) otherwise create a new attribute
object, register it to the table and return it. And it needs to do
it in a way that is thread-safe.
If we have to give access to it to the API users, then we'd need to
acquire and release the Big Attr Lock per each call.
The calls to git_attr() you need to make in your implementation will
be made from two codepaths:
* check_initl() acquires the Big Attr Lock, creates a check struct,
makes multiple calls to git_attr() to construct the necessary
git_attr instances to fill the array and then releases the lock,
so the git_attr() constructor does not have to be protected for
concurrent access.
* check_attr() acquires the Big Attr Lock, calls down to
prepare_attr_stack() as necessary to parse .gitattributes files
found in the directory hierarchy, which makes calls to git_attr()
to record the attributes found in the file. Then it does the
matching to fill results[] array and releases the lock. Again,
git_attr() constructors are called under the lock, so there is no
need for a separate lock.
If these are the only callpaths that reach git_attr() to construct
new attribute objects, it would mean that you can make this private
to attr subsystem and hide it from the users of the API.
Otherwise, you would need to rename the git_attr() constructor that
used internally under the Big Lock to
static struct git_attr *git_attr_locked(const char *);
that is defined inside attr.c, and then provide the external version
as a thin wrapper that calls it under the Big Lock, i.e.
struct git_attr *git_attr(const char *s)
{
struct git_attr *attr;
take_big_attr_lock();
attr = git_attr_locked(s);
release_big_attr_lock();
return attr;
}
That will have to make the big attr lock busier, and it would be
good if we can avoid it. That is where my "can we hide git_attr()
constructor?" comes from.
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Junio C Hamano @ 2016-10-27 5:46 UTC (permalink / raw)
To: Peter Williams; +Cc: Stefan Beller, Matt McCutchen, git
In-Reply-To: <f07745f8-d0ff-c41f-fd44-0812757fbd43@bigpond.net.au>
Peter Williams <pwil3058@bigpond.net.au> writes:
> However, for git commands such as diff/status whose job is to display
> information it would be nice if they had a --recursive option to
> override the default submodule diff/status and show details of the
> changes in the submodules. Sometimes you want to see the big picture
> in detail.
I won't disagree. My comment was only on this part from the original:
>> - We have to make separate commits and manage corresponding topic
>> branches for the superproject and subprojects.
and on this point, we seem to be in agreement.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 5:49 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes.Schindelin, git, simon, peff
In-Reply-To: <20161026215732.16411-1-sbeller@google.com>
Am 26.10.2016 um 23:57 schrieb Stefan Beller:
> In Windows it is not possible to have a static initialized mutex as of
> now, but that seems to be painful for the upcoming refactoring of the
> attribute subsystem, as we have no good place to put the initialization
> of the attr global lock.
Please rewrite the attribute system such that it can have a dynamic
initialization. If you find a global initialization in main() too gross
(I would agree) then setup_git_directory() might be the right place.
-- Hannes
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Junio C Hamano @ 2016-10-27 6:00 UTC (permalink / raw)
To: Peter Williams; +Cc: Stefan Beller, Matt McCutchen, git
In-Reply-To: <xmqqtwby8hu8.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Peter Williams <pwil3058@bigpond.net.au> writes:
>
>> However, for git commands such as diff/status whose job is to display
>> information it would be nice if they had a --recursive option to
>> override the default submodule diff/status and show details of the
>> changes in the submodules. Sometimes you want to see the big picture
>> in detail.
>
> I won't disagree. My comment was only on this part from the original:
>
>>> - We have to make separate commits and manage corresponding topic
>>> branches for the superproject and subprojects.
>
> and on this point, we seem to be in agreement.
Oh, and as Stefan mentioned, a "git diff" that recurses into the
submodules to give you detailed big picture has been in 'next'
(perhaps aready in 'master' as of tonight, but I am not sure
offhand) to be tested, together with many other fixes and
enhancements that all are waiting to be included in future releases.
The more people try and give feedback to these branches early, the
more solid release with better support for more goodies you'd want
we will be able to give you. Early adopters are always appreciated
but especially in time like this before the feature freeze for the
upcoming release (see tinyurl.com/gitCal for the schedule), they are
of great help.
Start by cloning from any one of these places
git://git.kernel.org/pub/scm/git/git.git/
https://kernel.googlesource.com/pub/scm/git/git
git://repo.or.cz/alt-git.git/
https://github.com/git/git/
and then
$ git checkout -b next origin/next
: read INSTALL to figure out if any custom options are needed
: in the following 'make' invocations for your environment
$ make && make install
$ PATH=$HOME/bin:$PATH
to join the fun.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-27 6:02 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Johannes.Schindelin, git, simon, peff
In-Reply-To: <93be5d21-6cb6-ee2b-9f4f-c2fe7c690d6c@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 26.10.2016 um 23:57 schrieb Stefan Beller:
>> In Windows it is not possible to have a static initialized mutex as of
>> now, but that seems to be painful for the upcoming refactoring of the
>> attribute subsystem, as we have no good place to put the initialization
>> of the attr global lock.
>
> Please rewrite the attribute system such that it can have a dynamic
> initialization. If you find a global initialization in main() too
> gross (I would agree) then setup_git_directory() might be the right
> place.
As many codepaths may not even need access to the attributes, I
doubt that would be a very productive direction to go.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 6:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, Johannes.Schindelin, git, simon, peff
In-Reply-To: <xmqqlgxa8h3a.fsf@gitster.mtv.corp.google.com>
Am 27.10.2016 um 08:02 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 26.10.2016 um 23:57 schrieb Stefan Beller:
>>> In Windows it is not possible to have a static initialized mutex as of
>>> now, but that seems to be painful for the upcoming refactoring of the
>>> attribute subsystem, as we have no good place to put the initialization
>>> of the attr global lock.
>>
>> Please rewrite the attribute system such that it can have a dynamic
>> initialization. If you find a global initialization in main() too
>> gross (I would agree) then setup_git_directory() might be the right
>> place.
>
> As many codepaths may not even need access to the attributes, I
> doubt that would be a very productive direction to go.
So, what is productive then? Pessimizing one (not exactly minor) platform?
-- Hannes
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-27 6:21 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Johannes.Schindelin, git, simon, peff
In-Reply-To: <67e38b43-0264-12f2-cca8-4b718ed7dc9d@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> As many codepaths may not even need access to the attributes, I
>> doubt that would be a very productive direction to go.
>
> So, what is productive then? Pessimizing one (not exactly minor) platform?
Lazy on-demand initialization as needed, perhaps? The on-demand
initialization mechanism may become no-op on some platforms that can
do static initialization.
^ permalink raw reply
* [PATCH] Update git rebase documentation to clarify HEAD behavior
From: Cody Sehl @ 2016-10-27 4:13 UTC (permalink / raw)
To: git
The first few paragraphs in the git-rebase.txt documentation lay out the steps git takes during a rebase:
1. everything from `<upstream>..HEAD` is saved to a temporary area
2. `HEAD` is set to `<upstream>`
3. the changes held in the temporary area are applied one by one in order on top of the new `HEAD`
The second step was described using the phrase `The current branch is reset to <upstream>`, which is true (because `HEAD` == current branch), but not clear.
---
Documentation/git-rebase.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index de222c8..c47ca11 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -33,7 +33,7 @@ of commits that would be shown by `git log <upstream>..HEAD`; or by
description on `--fork-point` below); or by `git log HEAD`, if the
`--root` option is specified.
-The current branch is reset to <upstream>, or <newbase> if the
+HEAD is reset to <upstream>, or <newbase> if the
--onto option was supplied. This has the exact same effect as
`git reset --hard <upstream>` (or <newbase>). ORIG_HEAD is set
to point at the tip of the branch before the reset.
--
https://github.com/git/git/pull/301
^ permalink raw reply related
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 6:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, Johannes.Schindelin, git, simon, peff
In-Reply-To: <xmqqh97y8g74.fsf@gitster.mtv.corp.google.com>
Am 27.10.2016 um 08:21 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> As many codepaths may not even need access to the attributes, I
>>> doubt that would be a very productive direction to go.
>>
>> So, what is productive then? Pessimizing one (not exactly minor) platform?
>
> Lazy on-demand initialization as needed, perhaps? The on-demand
> initialization mechanism may become no-op on some platforms that can
> do static initialization.
This is the pessimization that I am talking about. I would not mind at
all if it were only for the attribute subsystem, but the proposed patch
would pessimize *all* uses of pthread_mutex_lock.
-- Hannes
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-27 6:33 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Johannes.Schindelin, git, simon, peff
In-Reply-To: <xmqqh97y8g74.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> As many codepaths may not even need access to the attributes, I
>>> doubt that would be a very productive direction to go.
>>
>> So, what is productive then? Pessimizing one (not exactly minor) platform?
>
> Lazy on-demand initialization as needed, perhaps? The on-demand
> initialization mechanism may become no-op on some platforms that can
> do static initialization.
Ah, I think I misunderstood your "please rewrite". Did you mean to
add "void attr_start(void)" helper function to attr.c that does
series of pthread_mutex_init() calls as needed? That function can
be called from main() of platforms that cannot statically initialize
mutices, while on other platforms it can be a no-op as long as the
variables are statically initialized? If so, that would not pessimize
any platform, I would think.
^ permalink raw reply
* Re: [PATCH] Update git rebase documentation to clarify HEAD behavior
From: Junio C Hamano @ 2016-10-27 6:47 UTC (permalink / raw)
To: Cody Sehl; +Cc: git
In-Reply-To: <010201580457bdd2-99237b54-5e36-4430-bb8d-7e9088aed522-000000@eu-west-1.amazonses.com>
Cody Sehl <cody.sehl@gmail.com> writes:
> The first few paragraphs in the git-rebase.txt documentation lay out the steps git takes during a rebase:
> 1. everything from `<upstream>..HEAD` is saved to a temporary area
> 2. `HEAD` is set to `<upstream>`
> 3. the changes held in the temporary area are applied one by one in order on top of the new `HEAD`
>
> The second step was described using the phrase `The current branch is reset to <upstream>`, which is true (because `HEAD` == current branch), but not clear.
> ---
Please wrap your lines to reasonable lengths like 70 columns or so.
Please sign off your patch.
> Documentation/git-rebase.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index de222c8..c47ca11 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -33,7 +33,7 @@ of commits that would be shown by `git log <upstream>..HEAD`; or by
> description on `--fork-point` below); or by `git log HEAD`, if the
> `--root` option is specified.
>
> -The current branch is reset to <upstream>, or <newbase> if the
> +HEAD is reset to <upstream>, or <newbase> if the
> --onto option was supplied. This has the exact same effect as
> `git reset --hard <upstream>` (or <newbase>). ORIG_HEAD is set
> to point at the tip of the branch before the reset.
This is describing an ancient behaviour before 6fd2f5e60d ("rebase:
operate on a detached HEAD", 2007-11-08) in v1.5.4 timeframe. We
apparently failed to update the description.
This depends on the desired technical detail of the description, but
a correct rewrite would be "HEAD is detached at <upstream>, or
<newbase>, and ORIG_HEAD is set to point at the tip of the branch
before this happens". Detaching the HEAD at <upstream> no longer
has the same effect as "git reset --hard <upstream>" (or <newbase>),
so that sentence must go. It was the primary point of the ancient
change at 6fd2f5e60d after all.
And then there is a new step (to be numbered 4. in your description
in the proposed log message), which updates the tip of the branch to
the resulting HEAD (after replaying all these changes) and check the
branch out, which needs to be added. Perhaps after "one by one, in
order." Oh, the mention of "reapplied to the current branch" also
needs to be updated to "reapplied to the detached HEAD", too.
On the other hand, if we do not aim for that deep level of technical
correctness, but want to tell a white lie to make it easier to
understand at the conceptual level to new readers who haven't
grasped the detached HEAD, then the current description is fine. By
bringing up "HEAD", you seem to be aiming for techincal correctness
(which I tend to agree is a good direction to go in this part of the
documentation), so the existing text needs a bit more work than your
patch to be brought to the modern world.
^ 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