* Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files
From: Christian Couder @ 2016-10-27 12:14 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com>
On Thu, Oct 27, 2016 at 12:25 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> static int write_shared_index(struct index_state *istate,
>>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>>> }
>>> ret = rename_tempfile(&temporary_sharedindex,
>>> git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>>> - if (!ret)
>>> + if (!ret) {
>>> hashcpy(si->base_sha1, si->base->sha1);
>>> + clean_shared_index_files(sha1_to_hex(si->base->sha1));
>>
>> This operation is technically garbage collection and should belong to
>> "git gc --auto", which is already called automatically in a few
>> places. Is it not called often enough that we need to do the cleaning
>> up right after a new shared index is created?
>
> Christian, if we assume to go with Junio's suggestion to disable
> split-index on temporary files, the only files left we have to take
> care of are index and index.lock. I believe pruning here in this code
> will have an advantage over in "git gc --auto" because when this is
> executed, we know we're holding index.lock, so nobody else is updating
> the index, it's race-free. All we need to do is peek in $GIT_DIR/index
> to see what shared index file it requires and keep it alive too, the
> remaining of shared index files can be deleted safely. We don't even
> need to fall back to mtime.
>
> git-gc just can't match this because while it's running, somebody else
> may be updating $GIT_DIR/index. Handling races would be a lot harder.
Yeah, I agree that if we disable split-index on temporary files and on
commands like "git read-tree --index-output=<file>" then it is the
right thing to do it in write_shared_index(). (In fact when I wrote
the previous RFC series I didn't think about those special cases, and
that was the reason why I did it like this. So I just need to go back
to the implementation that was in the previous RFC series.)
I am just still wondering if disabling split-index on temporary files
could not have a bad performance impact for some use cases, but I
guess we could always come back to problem again if that happens.
Thanks,
Christian.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Alexei Lozovsky @ 2016-10-27 8:19 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <80919456-7563-2c16-ba23-ce4fcc2777de@pelly.co>
> 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.
I think the rc.d-like approach with directories is better as it
does not add new magical filenames (what if I absolutely do need
to name my directories ". path", with a space? :) keeping the
syntax of gitignores themselves simple, and allowing us to think
of files as still being separate entities. This can be useful
for the following case:
> 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.
As I understand, the precedence only matters for negative patterns
(the ones that start with an exclamation mark, !). For example,
suppose you have files 'foo1' and 'foo2'. This .gitignore
foo*
!foo1
will ignore foo2, but will show foo1. However, if the lines are
swapped:
!foo1
foo*
then both foo1 and foo2 will be ignored.
Now, if we consider the case of multiple .gitignore files, it
could be unexpected and possibly annoying for negative patterns
in one file to affect the patterns added by some other files.
I would find it more conceptually simple to apply individual
.gitignores one by one, as opposed to parsing them all and
creating one giant exclusion rule. (In technical terms, this
means keeping one struct exclude_list for each .gitignore,
not merging them all into one single list.)
In this case there should be no precendence problems as applied
gitignores only add new ignored files, without un-ignoring
anything previously ignored by other files.
However, if we allow textual inclusion, then it means that we
can put a gitignore into our gitignore so that we can unignore
while we ignore, which again brings us the question of whether
it is actually needed and expected.
> I would like to know the desirability/practicality/stupidity of such a
> feature as I believe it is within my skillset to implement it.
In my mind, this feature definitely has utility and it can be
implemented in backwards-compatible way, so why not.
However, I do not recall any precendent of git using rc.d-like
configs. And some can argue that your goal can be achieved by
generating the .gitignore by some external means and symlinking
the result into .git/info/exclude, so this is not Git's problem
and we should not be overcomplicating things with something as
simple as a list exclude patterns. This line of argument also
can be used to opposes any textual inclusion as well, because
it can be expanded into 'why don't we add a Turing-complete
programming language then to specify the patterns to ignore'.
^ permalink raw reply
* Re: [PATCH] Documentation/git-diff: document git diff with 3+ commits
From: Michael J Gruber @ 2016-10-27 9:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jacob Keller
In-Reply-To: <xmqqh97zask7.fsf@gitster.mtv.corp.google.com>
Junio C Hamano venit, vidit, dixit 26.10.2016 20:11:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> That one is difficult to discover but super useful, so document it:
>> Specifying 3 or more commits makes git diff switch to combined diff.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>
>> Notes:
>> Note that we have the following now:
>> ...
>> 'git diff A..B' equivalent to 'git diff A B'
>> in contrast to 'git log A..B' listing commits between M and B only
>> (without the commits between M and A unless they are "in" B).
>
> The standard answer is:
>
> Do not use two-dot form with 'git diff', if you find it
> confusing. Diff is about two endpoints, not about a range
> between two.
>
> The reason why we do not reject can be easily guessed by any
> intelligent person when some historical background is given, I
> think.
That is very well true. I'm more concerned with the presence, though,
that is: How easy to use is git now? Users choose git because they care
about the history of their project, not necessarily that of Git ;)
> - In the beginning A...B did not exist. A..B was the only "range"
> notation.
>
> - "git log A..B" was in wide use. Remember, "git log A...B" did
> not exist.
>
> - People started mistyping "git diff A..B", which looked as if the
> user typed "git diff ^A B" to the internal.
>
> - Git _could_ have rejected that as a bogus request to diff two
> points, ^A (what is that???) and B, but "What else could the user
> have meant with 'git diff A..B' other than 'git diff A B'?" was
> an argument to favor doing _something_ useful rather than
> erroring out. Remember, "A...B" did not exist when this
> happened.
It did not exist, but even at that point in time, "git log A..B" listed
only commits between the merge base and B, not those which are only in A
and not in B. Whereas "git diff A B" shows the differences between the
endpoints A and B.
>> @@ -12,6 +12,7 @@ SYNOPSIS
>> 'git diff' [options] [<commit>] [--] [<path>...]
>> 'git diff' [options] --cached [<commit>] [--] [<path>...]
>> 'git diff' [options] <commit> <commit> [--] [<path>...]
>> +'git diff' [options] <commit> <commit> <commit> [<commit>...]
>
> Made me wonder "is [<A>...] 0-or-more As or 1-or-more As?".
0-or-more, at least that's the way it is used in all lines here.
> Don't we allow pathspecs in this case?
Yes, the combinded diff mode kicks in only with no blobs (not: no
pathspec, which I had misread) and N>=3 commits. Maybe I should update
the code comments in builtin/diff.c to describe this, too.
Michael
^ 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
* 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] 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
* [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: 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
* 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: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: "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: 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 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: [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: 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 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: 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: 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: [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: "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: "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: [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: [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
* 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: 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
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