* password forgot
From: Luciano Schillagi @ 2016-10-25 15:52 UTC (permalink / raw)
To: git
Hi,
I forgot my password in git, such as resetting?
Thank You
Luciano
^ permalink raw reply
* [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Johannes Schindelin @ 2016-10-25 15:30 UTC (permalink / raw)
To: Eric Wong, git; +Cc: Gavin Lambert, Junio C Hamano
From: Gavin Lambert <github@mirality.co.nz>
Reusing cached data speeds up git-svn by quite a fair bit. However, if
the YAML module is unavailable, the caches are written to disk in an
architecture-dependent manner. That leads to problems when upgrading,
say, from 32-bit to 64-bit Git for Windows.
Let's just try to read those caches back if we detect the absence of the
YAML module and the presence of the file, and delete the file if it
could not be read back correctly.
Note that the only way to catch the error when the memoized cache could
not be read back is to put the call inside an `eval { ... }` block
because it would die otherwise; the `eval` block should also return `1`
in case of success explicitly since the function reading back the cached
data does not return an appropriate value to test for success.
This fixes https://github.com/git-for-windows/git/issues/233.
Signed-off-by: Gavin Lambert <github@mirality.co.nz>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/svn-multi-arch-v1
Fetch-It-Via: git fetch https://github.com/dscho/git svn-multi-arch-v1
We carried this for some time in Git for Windows.
perl/Git/SVN.pm | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..025c894 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
if ($memo_backend > 0) {
tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
} else {
+ # first verify that any existing file can actually be loaded
+ # (it may have been saved by an incompatible version)
+ if (-e "$path.db") {
+ unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
+ }
tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
}
}
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-10-25 15:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8DZWN0RRaDy9w2BVG5pn4FWCY=1YQDsP0V5obrr1wSzZQ@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:
> > diff --git a/environment.c b/environment.c
> > index cd5aa57..b1743e6 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -164,8 +164,11 @@ static void setup_git_env(void)
> > const char *replace_ref_base;
> >
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > - if (!git_dir)
> > + if (!git_dir) {
> > + if (!startup_info->have_repository)
> > + die("BUG: setup_git_env called without repository");
>
> YES!!! Thank you for finally fixing this.
Good, I'm glad somebody besides me is excited about this. I've been
wanting to write this patch for a long time, but it took years of
chipping away at all the edge cases.
> The "once we've identified" part could be tricky though. This message
> alone will not give us any clue where it's called since it's buried
> deep in git_path() usually, which is buried deep elsewhere. Without
> falling back to core dumps (with debug info), glibc's backtrace
> (platform specifc), the best we could do is turn git_path() into a
> macro that takes __FILE__ and __LINE__ and somehow pass the info down
> here, but "..." in macros is C99 specific, sigh..
>
> Is it too bad to turn git_path() into a macro when we know the
> compiler is C99 ? Older compilers will have no source location info in
> git_path(), Hopefully they are rare, which means chances of this fault
> popping up are also reduced.
I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.
But I think I still prefer just letting the corefile and the debugger do
their job. This error shouldn't happen much, and when it does, it should
be easily reproducible. Getting the bug reporter to give either a
reproduction recipe, or to run "gdb git" doesn't seem like that big a
hurdle.
For fun, here's a patch that uses backtrace(), but it does not actually
print the function names unless you compile with "-rdynamic" (and even
then it misses static functions). There are better libraries, but of
course that's one more thing for the user to deal with when building.
-Peff
---
diff --git a/usage.c b/usage.c
index 17f52c1b5c..4917c6bdfd 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,9 @@
*/
#include "git-compat-util.h"
#include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+#endif
static FILE *error_handle;
static int tweaked_error_buffering;
@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list params)
fputc('\n', fh);
}
+#ifdef HAVE_BACKTRACE
+static void maybe_backtrace(void)
+{
+ void *bt[100];
+ char **symbols;
+ int nr;
+
+ if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+ return;
+
+ nr = backtrace(bt, ARRAY_SIZE(bt));
+ symbols = backtrace_symbols(bt, nr);
+ if (symbols) {
+ FILE *fh = error_handle ? error_handle : stderr;
+ int i;
+
+ fprintf(fh, "die() called from:\n");
+ for (i = 0; i < nr; i++)
+ fprintf(fh, " %s\n", symbols[i]);
+ free(symbols);
+ }
+}
+#else
+#define maybe_backtrace()
+#endif
+
static NORETURN void usage_builtin(const char *err, va_list params)
{
vreportf("usage: ", err, params);
@@ -33,6 +62,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
static NORETURN void die_builtin(const char *err, va_list params)
{
vreportf("fatal: ", err, params);
+ maybe_backtrace();
exit(128);
}
^ permalink raw reply related
* Re: Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-25 15:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git
In-Reply-To: <CAHd499DKz+tpP3zRrXX3_serhoC_GCZst2y75JtC0Eiy1zfEew@mail.gmail.com>
On Wed, Oct 19, 2016 at 2:51 PM, Robert Dailey <rcdailey.lists@gmail.com> wrote:
> On Wed, Oct 19, 2016 at 2:45 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Oct 19, 2016 at 12:19 PM, Robert Dailey
>> <rcdailey.lists@gmail.com> wrote:
>>> On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller <sbeller@google.com> wrote:
>>>> You could try this patch series:
>>>> https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
>>>> (rebased to a newer version; no functional changes:)
>>>> https://github.com/stefanbeller/git/tree/submodule-co
>>>> (I'll rebase that later to origin/master)
>>>>
>>>>>
>>>>> Do you have any info on how I can prevent that error? Ideally I want
>>>>> the integration to go smoothly and transparently, not just for the
>>>>> person doing the actual transition (me) but for everyone else that
>>>>> gets those changes from upstream. They should not even notice that it
>>>>> happened (i.e. no failed commands, awkward behavior, or manual steps).
>>>>
>>>> It depends on how long you want to postpone the transition, but I plan to
>>>> upstream the series referenced above in the near future,
>>>> which would enable your situation to Just Work (tm). ;)
>>>
>>> At first glance, what you've linked to essentially looks like
>>> automated `git submodule update` for every `git checkout`. Am I
>>> misunderstanding?
>>
>> Essentially yes, except with stricter rules than the actual submodule update
>> IIRC.
>>
>>>
>>> If I'm correct, this is not the same as what I'm talking about. The
>>> problem appears to be more internal: When a submodule is removed, the
>>> physical files that were there are not removed by Git.
>>
>> That is also done by that series: submodules ought to be treated as files:
>> If you checkout a new version where a file is deleted, the checkout command
>> will actually remove the file for you (and e.g. solve any
>> directory/file conflicts
>> that may happen in the transition.)
>>
>>> It leaves them
>>> there in the working copy as untracked files.
>>
>> That is the current behavior as checkout tries hard to ignore submodules.
>>
>>> The next step Git takes
>>> (again, just from outside observation) is to add those very same files
>>> to the working copy, since they were added to a commit. However, at
>>> this point Git fails because it's trying to create (write) files to
>>> the working copy when an exact file of that name already exists there.
>>> Git will not overwrite untracked files, so at this point it fails.
>>>
>>> What needs to happen, somehow, is Git sees that the files were
>>> actually part of a submodule (which was removed) and remove the
>>> physical files as well, assuming that they were not modified in the
>>> submodule itself. This will ensure that the next step (creating the
>>> files) will succeed since the files no longer block it.
>>
>> Yep.
>
> It's great we're finally on the same page ;-)
>
> However, I don't see how this problem can be solved with your script,
> or solved in general outside of that. Does this mean that Git needs to
> change to treat submodules as it does normal files, per your previous
> assertion, which means submodules should *not* be left behind in the
> working copy as untracked files?
I'll assume (due to the lack of responses) that the only viable
solution here is to integrate the submodule using a different
directory name than the one used by the submodule itself. It's
unfortunate but I'll do it if I have no other option.
^ permalink raw reply
* Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository
From: Jeff King @ 2016-10-25 14:56 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8Bk32TcivD-5UO28UhdbpvCcxTE71cxFO2p_A4TZ1+GVw@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:24:50PM +0700, Duy Nguyen wrote:
> > Let's detect this situation explicitly and skip reading the
> > file (i.e., the same behavior we'd get if we were in a
> > repository and the file did not exist).
>
> On the other hand, if we invoke attr machinery too early by mistake,
> before setup_git_directory* is called, then we skip
> .git/info/attributes file as well even though I think we should shout
> "call setup_git_directory first!" so the developer can fix it.
> I wonder if we should have two flags in startup_info to say "yes
> setup_git_dir... has been called, you can trust
> startup_info->have_repository" and "yes, i do not call setup_git_dir
> on purpose, quit complaining" then we could still catch unintended
> .git/info/attributes ignore while letting git grep --no-index work
> correctly.
Yeah, it would be nice for the low-level code to be able to detect such
errors. I don't mind if you want to extend startup_info in that way, but
it will probably introduce a period of instability and regressions
(sites that are perfectly fine, but forgot to set the "I know what I'm
doing" flag).
-Peff
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Duy Nguyen @ 2016-10-25 13:02 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161025125856.p2paxt7erl2szptv@sigill.intra.peff.net>
On Tue, Oct 25, 2016 at 7:58 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
>
>> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
>> > compatible and less painful than using "%C(auto)" everywhere. I do
>> > wonder if anybody actually _wants_ the "always show color, even if
>> > --no-color" behavior. I'm having trouble thinking of a good use for it.
>> >
>> > IOW, I'm wondering if anyone would disagree that the current behavior is
>> > simply buggy.
>>
>> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
>> and turning the %(auto, no-op, for both log family and for-each-ref.
>> We could keep old behavior behind some environment variable if it's
>> not much work so it keeps working while people come here and tell us
>> about their use cases.
>
> Yeah, sorry.
Just to be clear (after re-reading my mail), the unwritten part after
"Silence in two weeks" was "so nobody disagreed with you", not "how is
your progress?". It's great that you keep working on this regardless
:D
--
Duy
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Jeff King @ 2016-10-25 12:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <CACsJy8B_AQxm1=vF8i4FPtinq0id1QZPrqp9vvAmAgUns_kgGg@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> > compatible and less painful than using "%C(auto)" everywhere. I do
> > wonder if anybody actually _wants_ the "always show color, even if
> > --no-color" behavior. I'm having trouble thinking of a good use for it.
> >
> > IOW, I'm wondering if anyone would disagree that the current behavior is
> > simply buggy.
>
> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
> and turning the %(auto, no-op, for both log family and for-each-ref.
> We could keep old behavior behind some environment variable if it's
> not much work so it keeps working while people come here and tell us
> about their use cases.
Yeah, sorry. I was blocked on making %(color:) in ref-filter work, which
required a bunch of refactoring in ref-filter, which conflicted heavily
with the kn/ref-filter-branch-list (which wants to do a lot of the same
things), and then I got blocked on reviewing that series (which overall
looks pretty sane, but I wanted to really dig in because I think it
hasn't gotten very careful review, or at least not recently).
So I'm still hoping to shave that yak at some point. Maybe this week.
-Peff
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Duy Nguyen @ 2016-10-25 12:52 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161010142818.lglwrxpks6l6aqrm@sigill.intra.peff.net>
On Mon, Oct 10, 2016 at 9:28 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:
>
>> > If we do a revamp of the pretty-formats to bring them more in line with
>> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
>> > opportunity to make minor adjustments. Though, hmm, it looks like
>> > for-each-ref already knows "%(color:red)", and it's unconditional.
>> > <sigh> So perhaps we would need to go through some deprecation period or
>> > other transition.
>>
>> We could add some new tag to change the behavior of all following %C
>> tags. Something like %C(tty) maybe (probably a bad name), then
>> discourage the use if "%C(auto" for terminal detection?
>
> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> compatible and less painful than using "%C(auto)" everywhere. I do
> wonder if anybody actually _wants_ the "always show color, even if
> --no-color" behavior. I'm having trouble thinking of a good use for it.
>
> IOW, I'm wondering if anyone would disagree that the current behavior is
> simply buggy.
Silence in two weeks. I vote (*) making %(<color-name>) honor --color
and turning the %(auto, no-op, for both log family and for-each-ref.
We could keep old behavior behind some environment variable if it's
not much work so it keeps working while people come here and tell us
about their use cases.
(*) I know.. voting is not how things work around here, unless you
vote with patches, but I can't take on another topic.
> Reading the thread at:
>
> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/
>
> I don't really see any compelling reason against it (there was some
> question of which config to use, but we already answered that with
> "%C(auto)", and use the value from the pretty_ctx).
>
> -Peff
--
Duy
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Duy Nguyen @ 2016-10-25 12:38 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net>
On Thu, Oct 20, 2016 at 1:24 PM, Jeff King <peff@peff.net> wrote:
> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite. So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> environment.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
> const char *replace_ref_base;
>
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir)
> + if (!git_dir) {
> + if (!startup_info->have_repository)
> + die("BUG: setup_git_env called without repository");
YES!!! Thank you for finally fixing this.
The "once we've identified" part could be tricky though. This message
alone will not give us any clue where it's called since it's buried
deep in git_path() usually, which is buried deep elsewhere. Without
falling back to core dumps (with debug info), glibc's backtrace
(platform specifc), the best we could do is turn git_path() into a
macro that takes __FILE__ and __LINE__ and somehow pass the info down
here, but "..." in macros is C99 specific, sigh..
Is it too bad to turn git_path() into a macro when we know the
compiler is C99 ? Older compilers will have no source location info in
git_path(), Hopefully they are rare, which means chances of this fault
popping up are also reduced.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository
From: Duy Nguyen @ 2016-10-25 12:24 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161020061641.3enppkoxfzr76has@sigill.intra.peff.net>
On Thu, Oct 20, 2016 at 1:16 PM, Jeff King <peff@peff.net> wrote:
> The low-level attribute and gitignore code will try to look
> in $GIT_DIR/info for any repo-level configuration files,
> even if we have not actually determined that we are in a
> repository (e.g., running "git grep --no-index"). In such a
> case they end up looking for ".git/info/attributes", etc.
>
> This is generally harmless, as such a file is unlikely to
> exist outside of a repository, but it's still conceptually
> the wrong thing to do.
>
> Let's detect this situation explicitly and skip reading the
> file (i.e., the same behavior we'd get if we were in a
> repository and the file did not exist).
On the other hand, if we invoke attr machinery too early by mistake,
before setup_git_directory* is called, then we skip
.git/info/attributes file as well even though I think we should shout
"call setup_git_directory first!" so the developer can fix it.
I wonder if we should have two flags in startup_info to say "yes
setup_git_dir... has been called, you can trust
startup_info->have_repository" and "yes, i do not call setup_git_dir
on purpose, quit complaining" then we could still catch unintended
.git/info/attributes ignore while letting git grep --no-index work
correctly.
--
Duy
^ permalink raw reply
* Re: [RFH] limiting ref advertisements
From: Duy Nguyen @ 2016-10-25 11:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net>
On Mon, Oct 24, 2016 at 8:29 PM, Jeff King <peff@peff.net> wrote:
> I'm looking into the oft-discussed idea of reducing the size of ref
> advertisements by having the client say "these are the refs I'm
> interested in". Let's set aside the protocol complexities for a
> moment and imagine we magically have some way to communicate a set of
> patterns to the server.
>
> What should those patterns look like?
>
> I had hoped that we could keep most of the pattern logic on the
> client-side. Otherwise we risk incompatibilities between how the client
> and server interpret a pattern. I had also hoped we could do some kind
> of prefix-matching, which would let the server look only at the
> interesting bits of the ref tree (so if you don't care about
> refs/changes, and the server has some ref storage that is hierarchical,
> they can literally get away without opening that sub-tree).
>
> The patch at the end of this email is what I came up with in that
> direction. It obviously won't compile without the twenty other patches
> implementing transport->advertise_prefixes
Yes! git-upload-pack-2 is making a come back, one form or another.
> but it gives you a sense of what I'm talking about.
>
> Unfortunately it doesn't work in all cases, because refspec sources may
> be unqualified. If I ask for:
>
> git fetch $remote master:foo
>
> then we have to actually dwim-resolve "master" from the complete list of
> refs we get from the remote. It could be "refs/heads/master",
> "refs/tags/master", etc. Worse, it could be "refs/master". In that case,
> at least, I think we are OK because we avoid advertising refs directly
> below "refs/" in the first place. But if you have a slash, like:
>
> git fetch $remote jk/foo
>
> then that _could_ be "refs/jk/foo". Likewise, we cannot even optimize
> the common case of a fully-qualified ref, like "refs/heads/foo". If it
> exists, we obviously want to use that. But if it doesn't, then it
> could be refs/something-else/refs/heads/foo. That's unlikely, but it
> _does_ work now, and optimizing the advertisement would break it.
>
> So it seems like left-anchoring the refspecs can never be fully correct.
> We can communicate "master" to the server, who can then look at every
> ref it would advertise and ask "could this be called master"? But it
> will be setting in stone the set of "could this be" patterns. Granted,
> those haven't changed much over the history of git, but it seems awfully
> fragile.
The first thought that comes to mind is, if left anchoring does not
work, let's support both left and right anchoring. I guess you
considered and discarded this.
If prefix matching does not work, and assuming "some-prefix" sent by
client to be in fact "**/some-prefix" pattern at server side will set
the "could this be" in stone, how about use wildmatch? It's flexible
enough and we have full control over the pattern matching engine so C
Git <-> C Git should be good regardless of platforms. I understand
that wildmatch is still complicated enough that a re-implementation
can easily divert in behavior. But a pattern with only '*', '/**',
'/**/' and '**/' wildcards (in other words, no [] or ?) could make the
engine a lot simpler and still fit our needs (and give some room for
client-optimization).
> In an ideal world the client and server would negotiate to come to some
> agreement on the patterns being used. But as we are bolting this onto
> the existing protocol, I was really trying to do it without introducing
> an extra capabilities phase or extra round-trips. I.e., something like
> David Turner's "stick the refspec in the HTTP query parameters" trick,
> but working everywhere[1].
--
Duy
^ permalink raw reply
* Re: [PATCH] doc: fix the 'revert a faulty merge' ASCII art tab spacing
From: Johannes Schindelin @ 2016-10-25 11:39 UTC (permalink / raw)
To: Philip Oakley; +Cc: gitster, git, peff
In-Reply-To: <20161024215432.1384-1-philipoakley@iee.org>
Hi Philip,
On Mon, 24 Oct 2016, Philip Oakley wrote:
> The asciidoctor doc-tool stack does not always respect the 'tab = 8 spaces' rule
> expectation, particularly for the Git-for-Windows generated html pages. This
> follows on from the 'doc: fix merge-base ASCII art tab spacing' fix.
>
> Use just spaces within the block of the ascii art.
>
> All other *.txt ascii art containing three dashes has been checked.
> Asciidoctor correctly formats the other art blocks that do contain tabs.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.org
> ---
> The git-scm doc pages https://git-scm.com/docs/ does not convert this
> how-to document to html, rather it links to the Github text pages, which
> does respect the 8 space tab rule.
I confirm that this fixes the misaligned branches when building the
documentation with asciidoctor 1.5.4 in Git for Windows' SDK.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks
From: Johannes Schindelin @ 2016-10-25 11:27 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, e, jnareb, gitster
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
Hi Lars,
On Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:
> This mini patch series is necessary to make the "ls/filter-process" topic
> work properly for Windows. Right now the topic generates test failures
> on Windows as reported by Dscho:
> http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/
I acknowledge that this fixes the test failures in Git for Windows' SDK.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Duy Nguyen @ 2016-10-25 11:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <CACsJy8B-GcMNv7pYYLpaUXc2kKnvyYEYm6w=fiaHy7rt4aug1Q@mail.gmail.com>
On Tue, Oct 25, 2016 at 6:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The procedure to resolve a merge conflict typically goes like this:
>>
>> - first open the file in the editor, and with the help of conflict
>> markers come up with a resolution.
>>
>> - save the file.
>>
>> - look at the output from "git diff" to see the combined diff to
>> double check if the resolution makes sense.
>>
>> - perform other tests, like trying to build the result with "make".
>>
>> - finally "git add file" to mark that you are done.
BTW making git-add (and "git commit -a") refuse files with conflict
markers present could prevent this mistake earlier and is probably a
better option because the user won't have to discover 'reset
--unmerge'. If the user likes to add the file with conflict markers
anyway (because those look like conflict markers but are in fact not)
then they can go with "git add -f" or similar.
--
Duy
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Duy Nguyen @ 2016-10-25 11:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqa8dttkbw.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The procedure to resolve a merge conflict typically goes like this:
>
> - first open the file in the editor, and with the help of conflict
> markers come up with a resolution.
>
> - save the file.
>
> - look at the output from "git diff" to see the combined diff to
> double check if the resolution makes sense.
>
> - perform other tests, like trying to build the result with "make".
>
> - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths. If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.
I made this exact mistake on a giant, half-resolved merge conflict the
other day and panicked. While I prepared myself to script update-index
to restore stages 2 and 3, I found "update-index --unresolve". It
sounds like this "reset --unmerge" is the same functionality, right?
I'm not objecting this patch though, update-index is not something a
casual user should use.
> There is "git checkout -m file" but that overwrites the working tree
> file to reproduce the conflicted state, which is not exactly what
> you want. You only want to reproduce the conflicted state in the
> index, so that you can inspect the (proposed) merge resolution you
> already have in your working tree.
--
Duy
^ permalink raw reply
* Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files
From: Duy Nguyen @ 2016-10-25 10:43 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-17-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> +static int can_delete_shared_index(const char *shared_sha1_hex)
> +{
> + struct stat st;
> + unsigned long expiration;
> + const char *shared_index = git_path("sharedindex.%s", shared_sha1_hex);
> +
> + /* Check timestamp */
> + expiration = get_shared_index_expire_date();
> + if (!expiration)
> + return 0;
> + if (stat(shared_index, &st))
> + return error_errno("could not stat '%s", shared_index);
> + if (st.st_mtime > expiration)
I wonder if we should check ctime too, in case mtime is not reliable
(and ctime is less likely to be manipulated by user), just for extra
safety. If (st.st_mtime > expiration || st.st_ctime > expiration).
> + return 0;
> +
> + return 1;
> +}
> +
> +static void clean_shared_index_files(const char *current_hex)
> +{
> + struct dirent *de;
> + DIR *dir = opendir(get_git_dir());
> +
> + if (!dir) {
> + error_errno("unable to open git dir: %s", get_git_dir());
_()
> + return;
Or just do "return error_errno(...)". The caller can ignore the return
value for now.
> + }
> +
> + while ((de = readdir(dir)) != NULL) {
> + const char *sha1_hex;
> + if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
> + continue;
> + if (!strcmp(sha1_hex, current_hex))
> + continue;
Yeah.. make sure that the shared index linked to $GIT_DIR/index stay,
even if mtime is screwed up. I wonder if we should have the same
treatment for $GIT_DIR/index.lock though, as an extra safety measure.
If you call this function in write_locked_index, then
$GIT_DIR/index.lock is definitely there. Hmm.. maybe it _is_
current_hex, current_hex is not the value from $GIT_DIR/index...
> + if (can_delete_shared_index(sha1_hex) > 0 &&
> + unlink(git_path("%s", de->d_name)))
> + error_errno("unable to unlink: %s", git_path("%s", de->d_name));
_()
> 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?
--
Duy
^ permalink raw reply
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Duy Nguyen @ 2016-10-25 10:52 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-1-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> Goal
> ~~~~
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.
Thanks. This definitely should help make split index a lot more
convenient for day-to-day use. The series looks ok (well, except the
pruning logic being discussed elsewhere in this thread, but I still
think it's a good strategy).
> This can be valuable as split-index can help significantly speed up
> `git rebase` especially along with the work to libify `git apply`
> that has been recently merged to master
> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).
I remember this. Since the apply libification work has landed, we can
now think about not writing index out after every apply step, which
gives the same (or more) speed up without the complication of
split-index. But if split-index is good enough for you, we can leave
it for another time.
--
Duy
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Johannes Schindelin @ 2016-10-25 10:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, larsxschneider, git, jnareb
In-Reply-To: <xmqqwpgx4j89.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 24 Oct 2016, Junio C Hamano wrote:
> Eric Wong <e@80x24.org> writes:
>
> > larsxschneider@gmail.com wrote:
> >> +++ b/read-cache.c
> >> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> >> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> >> {
> >> int match = -1;
> >> - int fd = open(ce->name, O_RDONLY);
> >> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> >> +
> >> + if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> >> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> >> + fd = open(ce->name, O_RDONLY);
> >
> > In the case of O_CLOEXEC != 0 and repeated EINVALs,
> > it'd be good to use something like sha1_file_open_flag as in 1/2
> > so we don't repeatedly hit EINVAL. Thanks.
>
> Sounds sane.
>
> It's just only once, so perhaps we do not mind a recursion like
> this?
>
> read-cache.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index b594865d89..a6978b9321 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> {
> int match = -1;
> - int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> + static int cloexec = O_CLOEXEC;
> + int fd = open(ce->name, O_RDONLY | cloexec);
>
> - if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
> /* Try again w/o O_CLOEXEC: the kernel might not support it */
> - fd = open(ce->name, O_RDONLY);
> + cloexec &= ~O_CLOEXEC;
> + return ce_compare_data(ce, st);
> + }
>
That still looks overly complicated, repeatedly ORing cloexec and
recursing without need. How about this instead?
static int oflags = O_RDONLY | O_CLOEXEC;
int fd = open(ce->name, oflags);
if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
oflags &= ~O_CLOEXEC;
fd = open(ce->name, oflags);
}
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC
From: Johannes Schindelin @ 2016-10-25 10:27 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, e, jnareb, gitster
In-Reply-To: <20161024180300.52359-2-larsxschneider@gmail.com>
Hi Lars,
On Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> All processes that the Git main process spawns inherit the open file
> descriptors of the main process. These leaked file descriptors can
> cause problems.
>
> Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
> descriptors. Since `git_open_noatime` does not describe the function
> properly anymore rename it to `git_open`.
The patch series may be a little bit more pleasant to read if you renamed
git_open_noatime() to git_open() first, in a separate commit.
> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
> if (fd >= 0)
> return fd;
>
> - /* Might the failure be due to O_NOATIME? */
> - if (errno != ENOENT && sha1_file_open_flag) {
> - sha1_file_open_flag = 0;
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + if (O_CLOEXEC && errno == EINVAL &&
> + (sha1_file_open_flag & O_CLOEXEC)) {
> + sha1_file_open_flag &= ~O_CLOEXEC;
How about
if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
sha1_file_open_flag &= ~O_CLOEXEC;
instead? It is shorter and should be just as easily optimized out by a
C compiler if O_CLOEXEC was defined as 0.
> continue;
> }
>
> + /* Might the failure be due to O_NOATIME? */
> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> + sha1_file_open_flag &= ~O_NOATIME;
> + continue;
> + }
I *think* the --patience diff option would have made that patch a little
more obvious.
Otherwise, the patch looks fine to me,
Dscho
^ permalink raw reply
* Re: [PATCH v1 14/19] read-cache: touch shared index files when used
From: Duy Nguyen @ 2016-10-25 10:26 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-15-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
Doing this in read_index_from() would keep the shared file even more
"fresher" since read happens a lot more often than write. But I think
our main concern is not the temporary index files created by the user
scripts, but $GIT_DIR/index.lock (make sure we don't accidentally
delete its shared file before it gets renamed to $GIT_DIR/index). For
this case, I think refreshing in write_locked_index is enough.
> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> + } else {
> + /* Signal that the shared index is used */
> + const char *shared_index = git_path("sharedindex.%s",
> + sha1_to_hex(si->base_sha1));
> + if (!check_and_freshen_file(shared_index, 1))
> + warning("could not freshen '%s'", shared_index);
_()
--
Duy
^ permalink raw reply
* Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
From: Duy Nguyen @ 2016-10-25 10:16 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-11-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
> if ((v & 15) < 6)
> istate->cache_changed |= SPLIT_INDEX_ORDERED;
> }
> - if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
> + if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
> + too_many_not_shared_entries(istate)) {
It's probably safer to keep this piece unchanged and add this
somewhere before it
if (too_many_not_shared_entries(istate))
istate->cache_changed |= SPLIT_INDEX_ORDERED;
We could keep cache_changed consistent until the end this way.
> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index db8c39f..507a1dd 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -8,6 +8,7 @@ test_description='split index mode tests'
> sane_unset GIT_TEST_SPLIT_INDEX
>
> test_expect_success 'enable split index' '
> + git config splitIndex.maxPercentChange 100 &&
An alternative name might be splitThreshold. I don't know, maybe
maxPercentChange is better.
> git update-index --split-index &&
> test-dump-split-index .git/index >actual &&
> indexversion=$(test-index-version <.git/index) &&
> --
> 2.10.1.462.g7e1e03a
>
--
Duy
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Duy Nguyen @ 2016-10-25 10:06 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-10-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> This new function will be used in a following commit to get the
> +int git_config_get_max_percent_split_change(void)
> +{
> + int val = -1;
> +
> + if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
> + if (0 <= val && val <= 100)
> + return val;
> +
> + error("splitindex.maxpercentchange value '%d' "
We should keep camelCase form for easy reading. And wrap this string with _().
> + "should be between 0 and 100", val);
I wonder if anybody would try to put 12.3 here and confused by the
error message, because 0 <= 12.3 <= 100, but it's not an integer..
Ah.. never mind, die_bad_number() would be called first in this case
with a loud and clear complaint.
> + return -1;
> + }
> +
> + return -1; /* default value */
--
Duy
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Duy Nguyen @ 2016-10-25 10:00 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-6-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> When users are using `git update-index --(no-)split-index`, they
> may expect the split-index feature to be used or not according to
> the option they just used, but this might not be the case if the
> new "core.splitIndex" config variable has been set. In this case
> let's warn about what will happen and why.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> builtin/update-index.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index b75ea03..a14dbf2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> }
>
> if (split_index > 0) {
> + if (git_config_get_split_index() == 0)
> + warning("core.splitIndex is set to false; "
> + "remove or change it, if you really want to "
> + "enable split index");
Wrap this string and the one below with _() so they can be translated.
> if (the_index.split_index)
> the_index.cache_changed |= SPLIT_INDEX_ORDERED;
> else
> add_split_index(&the_index);
> - } else if (!split_index)
> + } else if (!split_index) {
> + if (git_config_get_split_index() == 1)
> + warning("core.splitIndex is set to true; "
> + "remove or change it, if you really want to "
> + "disable split index");
> remove_split_index(&the_index);
> + }
>
> switch (untracked_cache) {
> case UC_UNSPECIFIED:
> --
> 2.10.1.462.g7e1e03a
--
Duy
^ permalink raw reply
* Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions
From: Duy Nguyen @ 2016-10-25 9:58 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <20161023092648.12086-4-chriscool@tuxfamily.org>
On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> +void remove_split_index(struct index_state *istate)
> +{
> + if (istate->split_index) {
> + /*
> + * can't discard_split_index(&the_index); because that
> + * will destroy split_index->base->cache[], which may
> + * be shared with the_index.cache[]. So yeah we're
> + * leaking a bit here.
In the context of update-index, this is a one-time thing and leaking
is tolerable. But because it becomes a library function now, this leak
can become more serious, I think.
The only other (indirect) caller is read_index_from() so probably not
bad most of the time (we read at the beginning of a command only).
sequencer.c may discard and re-read the index many times though,
leaking could be visible there.
> + */
> + istate->split_index = NULL;
> + istate->cache_changed |= SOMETHING_CHANGED;
> + }
> +}
--
Duy
^ permalink raw reply
* Re: [PATCH 0/4] nd/ita-empty-commit update
From: Duy Nguyen @ 2016-10-25 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqeg357hou.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 25, 2016 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The name ita-invisible-in-index is not perfect but I could not think
>> of any better. Another name could be diff-cached-ignores-ita, but
>> that's just half of what it does. The other half is diff-files-includes-ita...
>
> I can't either, and it is one of the reasons why I am reluctant.
> Not being able to be named with a short-and-sweet name often is a
> sign that the thing to be named is conceptually not well thought
> out.
It's implementation detail leak, and probably why naming it for
"normal" people is so hard. Whatever the name is must somehow imply
"so these i-t-a markers actually live in the index and considered real
index entries, associated to empty blob, most of the time..."
> But as we need to give it some name to the flat to ease
> experimenting, let's take that name as-is.
--
Duy
^ 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