* Re: `make profile-install` fails in 2.9.3
From: Jeff King @ 2016-09-01 21:58 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Jan Keromnes, git, Ingo Brückl, Edward Thomson
In-Reply-To: <20160901200700.GA8254@hank>
On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:
> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> > provide more debug information if you don't already know this problem.
>
> I noticed this problem as well, when I'm compiling with USE_NSEC = 1
> in my config.mak.
I can replicate this even without USE_NSEC with my stress-tester[1].
That makes sense why it would show up with the profiling run; git runs
slower and therefore increases the chances of crossing the 1-second
boundary and losing the race.
[1] https://github.com/peff/git/blob/meta/stress
> Tracking this problem down a bit, it happens because the --chmod=[+-]x
> option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
> options") only works if the file on disk is modified. When the test
> was changed to work on one single file, instead of doing chmod=+x on
> one file and chmod=-x on another file in b38ab197c ("t3700: merge two
> tests into one"), this test started breaking when the mtime of the
> file and the index file weren't the same (in other words, if the file
> was not racily clean and thus was not smudged).
That certainly sounds buggy. A less racy way of verifying this is just:
# guarantee not-racy state
echo content >file
test-chmtime -60 file
git add file
# now check --chmod; file will still be 100644!
git add --chmod=+x file
git ls-files -s
> One possible fix for the test is to smudge the entry as showed below,
> though I'm not sure it's the right fix. The other way I can think of
> is to change the file in the index regardless of whether the file was
> changed in some other way before issuing the git add command, as that
> might fit the user expectation better. Thoughts?
Yeah, I think we should _always_ act on the --chmod, no matter if the
file is racy or not, or whether it has a content change or not. I.e.,
the race is not the problem, but rather the behavior of 4e55ed32. Your
second proposal there sounds more like the right approach.
-Peff
^ permalink raw reply
* Re: git submodules implementation question
From: Uma Srinivasan @ 2016-09-01 19:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <xmqqa8frwhpr.fsf@gitster.mtv.corp.google.com>
>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers. A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.
Agreed!
The repo where the original problem surfaced is huge and in fact "git
status" appears to go into an infinite loop forking a zillion
processes on the system. If the dev system is small, it brings it to a
standstill. However, the good news is that I could build myself a
smaller reproducer by doing the following:
1) mkdir sm_test
2) cd sm_test
3) git clone git://git.mysociety.org/commonlib commonlib
4) git init
5) git submodule add ./commonlib/
6) cd commonlib/.git
7) rm -f all files
After this "git status" will fork several thousand processes but it
will ultimately fail as it runs into the path length max limit. I
still don't know why this doesn't happen with my original problem
repo. Perhaps I have to let it run overnight or perhaps it runs into
other system limitations before then....
Anyway, with the fix "git status" fails quickly both in my reproducer
(and original repo) with the following message.....
fatal: Not a git repository: '.git'
fatal: 'git status --porcelain' failed in submodule commonlib
Hope this helps.
Uma
On Thu, Sep 1, 2016 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers. A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.
>>
>> Tests are really good at providing this context as well, or to communicate
>> the actual underlying problem, which is not quite clear to me.
>> That is why I refrained from jumping into the discussion as I think the
>> first few emails were dropped from the mailing list and I am missing context.
>
> I do not know where you started reading, but the gist of it is that
> submodule.c spawns subprocess to run in the submodule's context by
> assuming that chdir'ing into the <path> of the submodule and running
> it (i.e. cp.dir set to <path> to drive start_command(&cp)) is
> sufficient. When <path>/.git (either it is a directory itself or it
> points at a directory in .git/module/<name> in the superproject) is
> a corrupt repository, running "git -C <path> command" would try to
> auto-detect the repository, because it thinks <path>/.git is not a
> repository and it thinks it is not at the top-level of the working
> tree, and instead finds the repository of the top-level, which is
> almost never what we want.
>
^ permalink raw reply
* Re: [PATCH] make dist: allow using an installed version of git
From: Dennis Kaarsemaker @ 2016-09-01 20:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4m5zy0qd.fsf@gitster.mtv.corp.google.com>
On do, 2016-09-01 at 10:43 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
> > b1de9de2 back in 2005 ensured that we could create a tarball with 'make
> > dist' even if git wasn't installed yet. These days however, chances are
> > higher that a git version is available. Add a config.mak knob to allow
> > people to choose to use the installed version of git to create the
> > tarball and avoid the overhead of building git-archive.
>
> Thanks, but not interested.
Pity. Would save me quite a bit of tarball build time.
> We do not know what vintage of "git" happens to be installed on the
> platform, but we know how "git archive" we ship with the source
> ought to behave.
That's why I didn't want to make it the default, but merely make it an
available option for saving some build time for people who know their
git is up-to-date enough.
D.
^ permalink raw reply
* Re: git submodules implementation question
From: Stefan Beller @ 2016-09-01 18:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Uma Srinivasan, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <xmqqmvjrwjwm.fsf@gitster.mtv.corp.google.com>
> The final version needs to be accompanied with tests to show the
> effect of this change for callers. A test would set up a top-level
> and submodule, deliberately break submodule/.git/ repository and
> show what breaks and how without this change.
Tests are really good at providing this context as well, or to communicate
the actual underlying problem, which is not quite clear to me.
That is why I refrained from jumping into the discussion as I think the
first few emails were dropped from the mailing list and I am missing context.
>
> So it is a bit more involved than just a single liner patch with one
> paragraph log message. I may be able to find or make some time
> after I tag the 2.10 final this weekend to do so myself.
Thanks,
Stefan
^ permalink raw reply
* Re: Should "git symbolic-ref -d HEAD" be forbidden?
From: Jeff King @ 2016-09-01 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpoonuy4n.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:
> I think we should.
>
> t1401 expects to be able to, but if you really do it:
>
> $ cd /tmp
> $ git init throwaway
> $ cd throwaway
> $ git symbolic-ref -d HEAD
>
> the setup machinery considers that you are no longer in a working
> tree that is controlled by a repository at .git/ because .git/ is
> no longer a valid repository, so you cannot even do
>
> $ git symbolic-ref HEAD refs/heads/master
>
> to recover.
Yes, I think we should, too. The same reasoning from afe5d3d (symbolic
ref: refuse non-ref targets in HEAD, 2009-01-29) applies.
-Peff
^ permalink raw reply
* Re: implement a stable 'Last updated' in Documentation
From: Jeff King @ 2016-09-01 20:17 UTC (permalink / raw)
To: Olaf Hering; +Cc: Michael J Gruber, Junio C Hamano, git
In-Reply-To: <20160901143736.GA26242@aepfle.de>
On Thu, Sep 01, 2016 at 04:37:36PM +0200, Olaf Hering wrote:
> Hey, asciidoc made a move, so this patch is good to go:
> https://github.com/asciidoc/asciidoc/pull/9
Sine this thread is 18 months old, I needed some recap to remember what
we were talking about. :)
It's here:
http://public-inbox.org/git/20150126172409.GA15204@aepfle.de/T/#u
and the gist of it is that we'd like to drop the "Last updated" footer
from the HTML version of the manpages, but older versions of asciidoc
did not provide a mechanism.
The patch you quoted adds "footer-style=none", which would do the trick.
But I have two open questions:
1. What does this do on older versions of asciidoc? Is it silently
ignored (ok), or does it generate an error (bad)?
2. This covers the HTML versions, but not the roff manpages (which
are generated by docbook). Do we have a way to tweak the date in
the latter?
I don't think that's necessarily a requirement for this patch, but
it is worth thinking about at the same time.
Assuming the answer to (1) is "ok" and (2) is "no, but it's hard because
docbook is scary, so let's punt", then somebody needs to write up the
commit message and send the actual patch to the list. Would you like to
try that?
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings
From: Jeff King @ 2016-09-01 20:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqvaygzpbe.fsf@gitster.mtv.corp.google.com>
On Wed, Aug 31, 2016 at 12:55:01PM -0700, Junio C Hamano wrote:
> Interesting. Here is for "gcc -Os" on top to appease gcc 4.8.4 that
> I probably am NOT going to apply. These are all false positives.
>
> The ones on config.c is the most curious as these two "ret" needs a
> false initialization, but the one that comes after them
> git_config_ulong() that has the same code structure does not get any
> warning, which made absolutely no sense to me.
Yeah, I'd agree that is really odd. I wondered if perhaps the signedness
of the argument mattered (e.g., if we were somehow provoking undefined
behavior which caused the compiler to make some assumption), but I just
don't see it.
> builtin/update-index.c | 2 +-
> config.c | 4 ++--
> diff.c | 2 +-
> fast-import.c | 1 +
> 4 files changed, 5 insertions(+), 4 deletions(-)
FWIW, all but the fast-import one have gone away in gcc 6.2.0 (using
-Os).
For that one:
> diff --git a/fast-import.c b/fast-import.c
> index bf53ac9..abc4519 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t *modep)
> unsigned char c;
> uint16_t mode = 0;
>
> + *modep = 0;
> while ((c = *str++) != ' ') {
> if (c < '0' || c > '7')
> return NULL;
The complaint actually comes from the caller, who doesn't realize that
modep will be set.
It pretty clearly seems to be a false positive, but I don't understand
it. If get_mode() is not inlined (or otherwise examined when considering
the caller), then it presumably should be treated as a block box that we
assume sets "modep". And if it is inlined, then it's pretty obvious
that "modep" is initialized in any code path that does not return NULL,
and we have:
p = get_mode(p, &mode);
if (!p)
die("Corrupt mode: %s", command_buf.buf);
in the caller (and "die" is marked as NORETURN). So it seems like a
pretty easy case to get right, and one that the compiler presumably gets
right elsewhere (otherwise we'd have a lot more of these false
positives).
Weird.
-Peff
^ permalink raw reply
* Re: Are --first-parent and --ancestry-path compatible rev-list options?
From: Junio C Hamano @ 2016-09-01 17:32 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List
In-Reply-To: <CD6AE25418644EB688D4488F8AB40155@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> From: "Junio C Hamano" <gitster@pobox.com>
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>>> The commit graph. We are looking for F based on knowing J.
>>>
>>> . A - B - C - D -- E -- F -- G - H <-first parent, --merges (C,F,H)
>>> . \ | / \ / /
>>> . ----Z | / /
>>> . | | | /
>>> . \ \ / /
>>> . I -[J]- K - L - M <-since J, children of J
>>> . \ /
>>> . N - O - P
>>
>> I think these two operations are fundamentally incompatible.
>
> If I run them independently, they both find the desired INTERESTED
> commit, hence the expectation that together they will still find that
> commit as an intersection between the two sets.
>
>>
>> Because the first-parent traversal is what the name says, i.e.,
>> forbids the positive side of revision traversal to stray into side
>> branches, the positive side of a traversal that begins at H will not
>> see M, L and K.
>
> But it does see F the ultimately desired commit.
You are doing --merges --first-parent, right? Traversing only the
first-parent chain on the positive side, while excluding J's
ancestor by traversing the negative side without being limited to
the first-parent chain, would paint B and its ancestors as
uninteresting on the first-parent chain, so among H, G, F, E, D and
C, which are the survivors on the first-parent chain that are still
not UNINTERESTIN, the last one you would find that is a merge is F.
So I do not see any room for "But" to come in here...
^ permalink raw reply
* Re: git submodules implementation question
From: Junio C Hamano @ 2016-09-01 18:32 UTC (permalink / raw)
To: Uma Srinivasan
Cc: Jacob Keller, Git Mailing List, Jens Lehmann, Heiko Voigt,
Stefan Beller
In-Reply-To: <CAN5XQftt3qVoU9gB2oyimY328VK0W6xq5FSCQYvcB9dEgkxVWA@mail.gmail.com>
Uma Srinivasan <usrinivasan@twitter.com> writes:
> Yes, this one line fix addresses my problem.
>
> So, what is the next step? Will someone submit a patch or should I?
> Please note that I've never submitted a patch before, but I don't mind
> learning how to.
The final version needs to be accompanied with tests to show the
effect of this change for callers. A test would set up a top-level
and submodule, deliberately break submodule/.git/ repository and
show what breaks and how without this change.
For example, there is a call in ok_to_remove_submodule() to run "git
status" in the submodule directory. A test writer needs to find who
calls ok_to_remove_submodule() in what codepath (e.g. builtin/rm.c).
Then after figuring out under what condition the check is triggered,
write a test that runs "git rm" in a way that this change makes a
difference. Hopefully, the test will fail without this change, and
will pass with this change.
So it is a bit more involved than just a single liner patch with one
paragraph log message. I may be able to find or make some time
after I tag the 2.10 final this weekend to do so myself.
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
>> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>> argv_array_push(out, *var);
>> }
>> + argv_array_push(out, "GIT_DIR=.git");
>> }
>>
^ permalink raw reply
* Re: git submodules implementation question
From: Junio C Hamano @ 2016-09-01 21:12 UTC (permalink / raw)
To: Stefan Beller
Cc: Uma Srinivasan, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <CAGZ79kaVBX_zLsSwkz=d_JUfvkeYHyZ1kkYm7Ae_dGBfFarDCQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> +test_expect_success 'fetching submodule into a broken repository' '
>> + # Prepare src and src/sub nested in it
>> + git init src &&
>> + (
>> + cd src &&
>> + git init sub &&
>> + git -C sub commit --allow-empty -m "initial in sub" &&
>> + git submodule add -- ./sub sub &&
>> + git commit -m "initial in top"
>> + ) &&
>
> This is not needed, as setup() set up some repositories for you.
I didn't want any random cruft left behind in the top-level by
previous tests, so this is very much deliberate.
^ permalink raw reply
* Re: git submodules implementation question
From: Stefan Beller @ 2016-09-01 21:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Uma Srinivasan, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <xmqq4m5zwevl.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 1, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>>> The final version needs to be accompanied with tests to show the
>>>> effect of this change for callers. A test would set up a top-level
>>>> and submodule, deliberately break submodule/.git/ repository and
>>>> show what breaks and how without this change.
>>>
>>> Tests are really good at providing this context as well, or to communicate
>>> the actual underlying problem, which is not quite clear to me.
>>> That is why I refrained from jumping into the discussion as I think the
>>> first few emails were dropped from the mailing list and I am missing context.
>>
>> I do not know where you started reading, but the gist of it is that
>> submodule.c spawns subprocess to run in the submodule's context by
>> assuming that chdir'ing into the <path> of the submodule and running
>> it (i.e. cp.dir set to <path> to drive start_command(&cp)) is
>> sufficient. When <path>/.git (either it is a directory itself or it
>> points at a directory in .git/module/<name> in the superproject) is
>> a corrupt repository, running "git -C <path> command" would try to
>> auto-detect the repository, because it thinks <path>/.git is not a
>> repository and it thinks it is not at the top-level of the working
>> tree, and instead finds the repository of the top-level, which is
>> almost never what we want.
>
> This is with a test that covers the call in get_next_submodule() for
> the parallel fetch callback. I think many of the codepaths will end
> up recursing forever the same way without the fix in a submodule
> repository that is broken in a similar way, but I didn't check, so
> I do not consider this to be completed.
Oh I see. That seems like a nasty bug.
>
> -- >8 --
> Subject: submodule: avoid auto-discovery in prepare_submodule_repo_env()
>
> The function is used to set up the environment variable used in a
> subprocess we spawn in a submodule directory. The callers set up a
> child_process structure, find the working tree path of one submodule
> and set .dir field to it, and then use start_command() API to spawn
> the subprocess like "status", "fetch", etc.
>
> When this happens, we expect that the ".git" (either a directory or
> a gitfile that points at the real location) in the current working
> directory of the subprocess MUST be the repository for the submodule.
>
> If this ".git" thing is a corrupt repository, however, because
> prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
> subprocess will see ".git", thinks it is not a repository, and
> attempt to find one by going up, likely to end up in finding the
> repository of the superproject. In some codepaths, this will cause
> a command run with the "--recurse-submodules" option to recurse
> forever.
>
> By exporting GIT_DIR=.git, disable the auto-discovery logic in the
> subprocess, which would instead stop it and report an error.
and GIT_DIR=.git works for both .git files as well as the old fashioned way,
with the submodule repository at .git/, although that is not really documented.
>
> Not-signed-off-yet.
> ---
> submodule.c | 1 +
> t/t5526-fetch-submodules.sh | 29 +++++++++++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb..e8258f0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
> argv_array_push(out, *var);
> }
> + argv_array_push(out, "GIT_DIR=.git");
> }
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 954d0e4..b2dee30 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -485,4 +485,33 @@ test_expect_success 'fetching submodules respects parallel settings' '
> )
> '
>
> +test_expect_success 'fetching submodule into a broken repository' '
> + # Prepare src and src/sub nested in it
> + git init src &&
> + (
> + cd src &&
> + git init sub &&
> + git -C sub commit --allow-empty -m "initial in sub" &&
> + git submodule add -- ./sub sub &&
> + git commit -m "initial in top"
> + ) &&
This is not needed, as setup() set up some repositories for you.
> +
> + # Clone the old-fashoned way
> + git clone src dst &&
if you don't go with your own setup, maybe:
git clone . dst
git -C dst clone ../submodule submodule
# and further down s/sub/submodule/
> + git -C dst clone ../src/sub sub &&
> +
> + # Make sure that old-fashoned layout is still supported
fashioned
> + git -C dst status &&
> +
> + # Recursive-fetch works fine
> + git -C dst fetch --recurse-submodules &&
> +
> + # Break the receiving submodule
> + rm -f dst/sub/.git/HEAD &&
> +
> + # Recursive-fetch must terminate
> + # NOTE: without fix this will recurse forever!
> + test_must_fail git -C dst fetch --recurse-submodules
So in case we'd break it again in the future we'd notice by an
infinite time for the test suite, then we'd notice this comment and
know what's going on?
I would have suggested to run this actual test in a subshell and then
check if the expected failure would have recovered after a second or two
and depending on that trigger a test failure.
But it doesn't seem to be easy with shells, so this is fine (even the newer
signed off patch, apart from nits above)
> +'
> +
> test_done
>
>
^ permalink raw reply
* Should "git symbolic-ref -d HEAD" be forbidden?
From: Junio C Hamano @ 2016-09-01 21:08 UTC (permalink / raw)
To: git
I think we should.
t1401 expects to be able to, but if you really do it:
$ cd /tmp
$ git init throwaway
$ cd throwaway
$ git symbolic-ref -d HEAD
the setup machinery considers that you are no longer in a working
tree that is controlled by a repository at .git/ because .git/ is
no longer a valid repository, so you cannot even do
$ git symbolic-ref HEAD refs/heads/master
to recover.
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Junio C Hamano @ 2016-09-01 18:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narębski, git
In-Reply-To: <alpine.DEB.2.20.1609011052260.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> though). The "one sequencer to rule them all" may even have to say
>> "now give name ':1' to the result of the previous operation" in one
>> step and in another later step have an instruction "merge ':1'".
>> When that happens, you cannot even pre-populate the commit object
>> when the sequencer reads the file, as the commit has not yet been
>> created at that point.
>
> These considerations are pretty hypothetical. I would even place a bet
> that we will *never* have ":1" as names, not if I have anything to say...
> ;-)
If you can always work with pre-existing commit, then you can
validate all object references that appear in the instructions
upfront.
I was sort of expecting that, when you do the preserve-merges mode
of "rebase -i", you would need to jump around, doing "we have
reconstructed the side branch on a new 'onto', let's give the result
this temporary name ':1', and then switch to the trunk (which would
call for 'reset <commit>' instruction) and merge that thing (which
would be 'merge :1' or perhaps called 'pick :1')", and at that point
you no longer validate the object references upfront.
If you do not have to have such a "mark this point" and a "refer to
that point we previously marked", then I agree that you should be
able to pre-validate and keep the result in the structure.
^ permalink raw reply
* Re: git submodules implementation question
From: Junio C Hamano @ 2016-09-01 20:29 UTC (permalink / raw)
To: Uma Srinivasan
Cc: Stefan Beller, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <CAN5XQft1YFEdxcB8Q_qFG4iYmSaSX_JXDCESzeEyYqPO0BUbaw@mail.gmail.com>
Uma Srinivasan <usrinivasan@twitter.com> writes:
> Anyway, with the fix "git status" fails quickly both in my reproducer
> (and original repo) with the following message.....
> fatal: Not a git repository: '.git'
> fatal: 'git status --porcelain' failed in submodule commonlib
Thanks, that is exactly what I wanted to see as an outcome when I
did the one-liner patch.
^ permalink raw reply
* Re: [PATCH v13 00/14] libify apply and use lib in am, part 3
From: Junio C Hamano @ 2016-09-01 17:48 UTC (permalink / raw)
To: Christian Couder
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Karsten Blees, Nguyen Thai Ngoc Duy, Stefan Beller, Eric Sunshine,
Ramsay Jones, Johannes Sixt, René Scharfe, Stefan Naewe,
Christian Couder
In-Reply-To: <CAP8UFD3wA32eXYq3F4=KS-9SkV48Yh45TKgFnn3AmGVfpjwWjA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> Following Stefan's review, it looks like I will need to resend at
> least 02/14, 10/14 and 14/14.
> What do you prefer me to resend:
> 1) all the last 40 or so patches
> 2) the last 14 patches
> 3) only the few patches that changed
If this reroll is to be the candidate to be the final one, I'd
prefer to see 1 to give an easy access to more sets of eyes, but if
you just send them without giving these patches one more read-over
before sending them out, it is not as valuable as it would be.
I think 2 is of the least value.
If you do 3., pointing people at where the remainder of the series
can be found is necessary.
^ permalink raw reply
* Re: git submodules implementation question
From: Junio C Hamano @ 2016-09-01 21:02 UTC (permalink / raw)
To: Stefan Beller
Cc: Uma Srinivasan, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <xmqq4m5zwevl.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
If we add
# "git diff" should terminate with an error.
# NOTE: without fix this will recurse forever!
test_must_fail git -C dst diff &&
after breaking the repository, we can also see "git diff" recurse
forever, because it wants to know if "sub" submodule is modified,
attempts to run "git status" in there, and ends up running that
command in the context of the parent repository.
I am tempted to cheat and commit this, even though this test is no
longer about fetching submodules.
-- >8 --
[PATCH] submodule: avoid auto-discovery in prepare_submodule_repo_env()
The function is used to set up the environment variable used in a
subprocess we spawn in a submodule directory. The callers set up a
child_process structure, find the working tree path of one submodule
and set .dir field to it, and then use start_command() API to spawn
the subprocess like "status", "fetch", etc.
When this happens, we expect that the ".git" (either a directory or
a gitfile that points at the real location) in the current working
directory of the subprocess MUST be the repository for the submodule.
If this ".git" thing is a corrupt repository, however, because
prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
subprocess will see ".git", thinks it is not a repository, and
attempt to find one by going up, likely to end up in finding the
repository of the superproject. In some codepaths, this will cause
a command run with the "--recurse-submodules" option to recurse
forever.
By exporting GIT_DIR=.git, disable the auto-discovery logic in the
subprocess, which would instead stop it and report an error.
The test illustrates existing problems in a few callsites of this
function. Without this fix, "git fetch --recurse-submodules", "git
status" and "git diff" keep recursing forever.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule.c | 1 +
t/t5526-fetch-submodules.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/submodule.c b/submodule.c
index 4532b11..2801fbb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
+ argv_array_push(out, "GIT_DIR=.git");
}
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 954d0e4..f3b0a8d 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -485,4 +485,39 @@ test_expect_success 'fetching submodules respects parallel settings' '
)
'
+test_expect_success 'fetching submodule into a broken repository' '
+ # Prepare src and src/sub nested in it
+ git init src &&
+ (
+ cd src &&
+ git init sub &&
+ git -C sub commit --allow-empty -m "initial in sub" &&
+ git submodule add -- ./sub sub &&
+ git commit -m "initial in top"
+ ) &&
+
+ # Clone the old-fashoned way
+ git clone src dst &&
+ git -C dst clone ../src/sub sub &&
+
+ # Make sure that old-fashoned layout is still supported
+ git -C dst status &&
+
+ # "diff" would find no change
+ git -C dst diff --exit-code &&
+
+ # Recursive-fetch works fine
+ git -C dst fetch --recurse-submodules &&
+
+ # Break the receiving submodule
+ rm -f dst/sub/.git/HEAD &&
+
+ # NOTE: without the fix the following tests will recurse forever!
+ # They should terminate with an error.
+
+ test_must_fail git -C dst status &&
+ test_must_fail git -C dst diff &&
+ test_must_fail git -C dst fetch --recurse-submodules
+'
+
test_done
--
2.10.0-rc2-314-g775ea9a
^ permalink raw reply related
* Re: [PATCH] make dist: allow using an installed version of git
From: Junio C Hamano @ 2016-09-01 17:43 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git
In-Reply-To: <20160827102929.GA11769@hurricane>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> b1de9de2 back in 2005 ensured that we could create a tarball with 'make
> dist' even if git wasn't installed yet. These days however, chances are
> higher that a git version is available. Add a config.mak knob to allow
> people to choose to use the installed version of git to create the
> tarball and avoid the overhead of building git-archive.
Thanks, but not interested.
We do not know what vintage of "git" happens to be installed on the
platform, but we know how "git archive" we ship with the source
ought to behave.
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
> Makefile | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d96ecb7..3dabb75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -378,6 +378,9 @@ all::
> #
> # to say "export LESS=FRX (and LV=-c) if the environment variable
> # LESS (and LV) is not set, respectively".
> +#
> +# Define USE_INSTALLED_GIT_ARCHIVE if you don't want to build git-archive as
> +# part of 'make dist', but are happy to rely on a git version on you $PATH
>
> GIT-VERSION-FILE: FORCE
> @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -2423,8 +2426,15 @@ quick-install-html:
> ### Maintainer's dist rules
>
> GIT_TARNAME = git-$(GIT_VERSION)
> -dist: git-archive$(X) configure
> - ./git-archive --format=tar \
> +ifndef USE_INSTALLED_GIT_ARCHIVE
> + GIT_ARCHIVE = ./git-archive$(X)
> + GIT_ARCHIVE_DEP = git-archive$(X)
> +else
> + GIT_ARCHIVE = git archive
> + GIT_ARCHIVE_DEP =
> +endif
> +dist: $(GIT_ARCHIVE_DEP) configure
> + $(GIT_ARCHIVE) --format=tar \
> --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
> @mkdir -p $(GIT_TARNAME)
> @cp configure $(GIT_TARNAME)
^ permalink raw reply
* Re: git submodules implementation question
From: Junio C Hamano @ 2016-09-01 20:21 UTC (permalink / raw)
To: Stefan Beller
Cc: Uma Srinivasan, Jacob Keller, Git Mailing List, Jens Lehmann,
Heiko Voigt
In-Reply-To: <xmqqa8frwhpr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers. A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.
>>
>> Tests are really good at providing this context as well, or to communicate
>> the actual underlying problem, which is not quite clear to me.
>> That is why I refrained from jumping into the discussion as I think the
>> first few emails were dropped from the mailing list and I am missing context.
>
> I do not know where you started reading, but the gist of it is that
> submodule.c spawns subprocess to run in the submodule's context by
> assuming that chdir'ing into the <path> of the submodule and running
> it (i.e. cp.dir set to <path> to drive start_command(&cp)) is
> sufficient. When <path>/.git (either it is a directory itself or it
> points at a directory in .git/module/<name> in the superproject) is
> a corrupt repository, running "git -C <path> command" would try to
> auto-detect the repository, because it thinks <path>/.git is not a
> repository and it thinks it is not at the top-level of the working
> tree, and instead finds the repository of the top-level, which is
> almost never what we want.
This is with a test that covers the call in get_next_submodule() for
the parallel fetch callback. I think many of the codepaths will end
up recursing forever the same way without the fix in a submodule
repository that is broken in a similar way, but I didn't check, so
I do not consider this to be completed.
-- >8 --
Subject: submodule: avoid auto-discovery in prepare_submodule_repo_env()
The function is used to set up the environment variable used in a
subprocess we spawn in a submodule directory. The callers set up a
child_process structure, find the working tree path of one submodule
and set .dir field to it, and then use start_command() API to spawn
the subprocess like "status", "fetch", etc.
When this happens, we expect that the ".git" (either a directory or
a gitfile that points at the real location) in the current working
directory of the subprocess MUST be the repository for the submodule.
If this ".git" thing is a corrupt repository, however, because
prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
subprocess will see ".git", thinks it is not a repository, and
attempt to find one by going up, likely to end up in finding the
repository of the superproject. In some codepaths, this will cause
a command run with the "--recurse-submodules" option to recurse
forever.
By exporting GIT_DIR=.git, disable the auto-discovery logic in the
subprocess, which would instead stop it and report an error.
Not-signed-off-yet.
---
submodule.c | 1 +
t/t5526-fetch-submodules.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/submodule.c b/submodule.c
index 1b5cdfb..e8258f0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
+ argv_array_push(out, "GIT_DIR=.git");
}
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 954d0e4..b2dee30 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -485,4 +485,33 @@ test_expect_success 'fetching submodules respects parallel settings' '
)
'
+test_expect_success 'fetching submodule into a broken repository' '
+ # Prepare src and src/sub nested in it
+ git init src &&
+ (
+ cd src &&
+ git init sub &&
+ git -C sub commit --allow-empty -m "initial in sub" &&
+ git submodule add -- ./sub sub &&
+ git commit -m "initial in top"
+ ) &&
+
+ # Clone the old-fashoned way
+ git clone src dst &&
+ git -C dst clone ../src/sub sub &&
+
+ # Make sure that old-fashoned layout is still supported
+ git -C dst status &&
+
+ # Recursive-fetch works fine
+ git -C dst fetch --recurse-submodules &&
+
+ # Break the receiving submodule
+ rm -f dst/sub/.git/HEAD &&
+
+ # Recursive-fetch must terminate
+ # NOTE: without fix this will recurse forever!
+ test_must_fail git -C dst fetch --recurse-submodules
+'
+
test_done
^ permalink raw reply related
* Re: `make profile-install` fails in 2.9.3
From: Thomas Gummerer @ 2016-09-01 20:07 UTC (permalink / raw)
To: Jan Keromnes; +Cc: git, Ingo Brückl, Edward Thomson
In-Reply-To: <CAA6PgK7C18F1WGyZMTEUAWEVsUWqiZND5Ne_0SH-rUEm8u5dNg@mail.gmail.com>
[+cc Edward Thomson, Ingo Brückl]
Hi,
On 09/01, Jan Keromnes wrote:
[.. snip the parts others are more qualified to answer ..]
>
> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.
I noticed this problem as well, when I'm compiling with USE_NSEC = 1
in my config.mak.
Tracking this problem down a bit, it happens because the --chmod=[+-]x
option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
options") only works if the file on disk is modified. When the test
was changed to work on one single file, instead of doing chmod=+x on
one file and chmod=-x on another file in b38ab197c ("t3700: merge two
tests into one"), this test started breaking when the mtime of the
file and the index file weren't the same (in other words, if the file
was not racily clean and thus was not smudged).
When the file is racily clean, git detects that the contents changed,
and everything is happy, but if it isn't, git incorectly thinks the
file wasn't modified (which it wasn't, but from gits view it should be
viewed as modified because the mode does not match up anymore).
One possible fix for the test is to smudge the entry as showed below,
though I'm not sure it's the right fix. The other way I can think of
is to change the file in the index regardless of whether the file was
changed in some other way before issuing the git add command, as that
might fit the user expectation better. Thoughts?
diff --git a/read-cache.c b/read-cache.c
index 491e52d..f2e7986 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -656,11 +656,13 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
else
ce->ce_flags |= CE_INTENT_TO_ADD;
- if (S_ISREG(st_mode) && force_mode)
+ if (S_ISREG(st_mode) && force_mode) {
ce->ce_mode = create_ce_mode(force_mode);
- else if (trust_executable_bit && has_symlinks)
+ ce->ce_stat_data.sd_size = 0;
+ } else if (trust_executable_bit && has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
- else {
+ } else {
/* If there is an existing entry, pick the mode bits and type
* from it, otherwise assume unexecutable regular file.
*/
> Thanks,
> Jan Keromnes
>
> ---
>
> Steps to reproduce:
>
> curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \
> && cd git-2.9.3 \
> && make prefix=/usr profile-install install-man -j18
>
> Expected result:
>
> - runs all tests to get a profile (ignoring occasional failures)
> - rebuilds Git with the profile
> - installs Git
>
> Actual result:
>
> - runs all tests to get a profile
> - at least one test fails, interrupting the whole process
> - Git is not installed
>
> Failure log:
>
> # failed 1 among 40 test(s)
> 1..40
> Makefile:43: recipe for target 't3700-add.sh' failed
> make[3]: *** [t3700-add.sh] Error 1
> make[3]: Leaving directory '/tmp/git/git-2.9.3/t'
> Makefile:36: recipe for target 'test' failed
> make[2]: Leaving directory '/tmp/git/git-2.9.3/t'
> make[2]: *** [test] Error 2
> Makefile:2221: recipe for target 'test' failed
> make[1]: *** [test] Error 2
> make[1]: Leaving directory '/tmp/git/git-2.9.3'
> Makefile:1633: recipe for target 'profile' failed
> make: *** [profile] Error 2
> The command '/bin/sh -c mkdir /tmp/git && cd /tmp/git && curl
> https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ
> && cd git-2.9.3 && make prefix=/usr profile-install install-man -j18
> && rm -rf /tmp/git' returned a non-zero code: 2
--
Thomas
^ permalink raw reply related
* Re: [PATCH 08/22] sequencer: remove overzealous assumption
From: Jakub Narębski @ 2016-09-01 20:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609010957300.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 10:01, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
>>
>> I wonder: would 'git cherry-pick --continue' be able to finish
>> 'git revert', and vice versa, then? Or 'git sequencer --continue'?
>
> I just tested this, via
>
> diff --git a/t/t3510-cherry-pick-sequence.sh
> b/t/t3510-cherry-pick-sequence.sh
> index 96c7640..085d8bc 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -55,7 +55,7 @@ test_expect_success 'cherry-pick
> mid-cherry-pick-sequence' '
> git checkout HEAD foo &&
> git cherry-pick base &&
> git cherry-pick picked &&
> - git cherry-pick --continue &&
> + git revert --continue &&
> git diff --exit-code anotherpick
>
> (Danger! Whitespace corrupted!!!)
>
> It appears that this passes now.
I'm now not sure if it is such a great idea. As was said somewhere else
in this thread, different sequencer-based commands sports different
options, and you can add options to the "git <command> --continue".
For example you can say "git cherry-pick --continue -x", but you
cannot say "git revert --continue -x", as '-x' is a cherry-pick only
option. Or you can, theoretically, use "git am --continue --no-3way".
One option is to temporarily relax the test (test_expect_failure),
then fix it at the end.
BTW. how git-am uses sequencer? I have seen "revert" etc., and "pick"
etc., but no git-am related constants or strings...
> Probably `git sequencer --continue` would work, too, if there was a `git
> sequencer`. :0)
Right.
>
>>> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>>>> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>>
>>>>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>>>>> index 7b7a89d..6465edf 100755
>>>>> --- a/t/t3510-cherry-pick-sequence.sh
>>>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>>>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>>>>> test_expect_code 128 git cherry-pick --continue
>>>>> '
>>>>>
>>>>> -test_expect_success 'malformed instruction sheet 2' '
>>>>
>>>> Hmmm... the description is somewhat lacking (especially compared to
>>>> the rest of test), anyway.
>>>>
>>>> BTW. we should probably rename 'malformed instruction sheet 2'
>>>> to 'malformed instruction sheet' if there are no further such
>>>> tests after this removal, isn't it?
>>>
>>> No, we cannot rename it after this patch because the patch removes it ;-)
>>> (It is not a file name but really a label for a test case.)
>>
>> Ooops. What I wanted to say that after removing the test case named
>> 'malformed instruction sheet 2' we should also rename *earlier* test
>> case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
>> as it is now the only 'malformed instruction sheet *' test case.
>
> Actually, you know, I completely missed the fact that there was a
> "malformed instruction sheet 3". I renumbered it.
Ooops. I have missed it too, having looked only at the test after the
one removed (which is not about malformed instruction sheet).
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: Are --first-parent and --ancestry-path compatible rev-list options?
From: Philip Oakley @ 2016-09-01 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqq8tvby19a.fsf@gitster.mtv.corp.google.com>
Hi Junio,
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> From: "Junio C Hamano" <gitster@pobox.com>
>>> "Philip Oakley" <philipoakley@iee.org> writes:
>>>
>>>> The commit graph. We are looking for F based on knowing J.
>>>>
>>>> . A - B - C - D -- E -- F -- G - H <-first parent, --merges (C,F,H)
>>>> . \ | / \ / /
>>>> . ----Z | / /
>>>> . | | | /
>>>> . \ \ / /
>>>> . I -[J]- K - L - M <-since J, children of J
>>>> . \ /
>>>> . N - O - P
>>>
>>> I think these two operations are fundamentally incompatible.
>>
>> If I run them independently, they both find the desired INTERESTED
>> commit, hence the expectation that together they will still find that
>> commit as an intersection between the two sets.
>>
>>>
>>> Because the first-parent traversal is what the name says, i.e.,
>>> forbids the positive side of revision traversal to stray into side
>>> branches, the positive side of a traversal that begins at H will not
>>> see M, L and K.
>>
>> But it does see F the ultimately desired commit.
>
> You are doing --merges --first-parent, right? Traversing only the
> first-parent chain on the positive side, while excluding J's
> ancestor by traversing the negative side without being limited to
> the first-parent chain, would paint B and its ancestors as
> uninteresting on the first-parent chain, so among H, G, F, E, D and
> C, which are the survivors on the first-parent chain that are still
> not UNINTERESTIN, the last one you would find that is a merge is F.
>
> So I do not see any room for "But" to come in here...
>
The confusion is between the "As required" and "as coded" viewpoints (this
is regular dayjob problem of allegedly having 'requirement specifications').
You have rightly described the algorithm as currently implemented, while I
was was trying to state a requirement (based on a user question).
The user question was, given a commit 'J', and a future commit 'H'
(typically a branch tip such as 'master'), find those commits that are :
A) merges
B) on the first parent DAG chain of the future commit 'H'
C) children of the given commit 'J'
i.e. the points on master where the feature J (and it's children) could have
brought in some effect to master.
Each of the three conditions match one of the revision list options, but the
implemented logic does not produce the AND effect that could be expected.
Essentially it (the user's desire vs the coding implementation) is a case
where (depending on the viewpoint) we get the 'denying the antecedent'
fallacy.
Just because a commit is not --first-parent does not mean that revison
walking (in this case) should stop. The user is interested in the ancestry
path, so from that perspective the the walk should carry on through the
other parents till its reaches the TRULY_DISINTERESTED line, or reaches J.
With the current implementation one appears to need to script a search
through all the first parent merges and then prune out those that aren't
merge-base is-ancestor commits, which is what the OP ended up having to do.
I haven't had any time to look into the rev-walk code itself, but is this
something that could be coded (new option) or is the double-duty problem
with UNINTERESTING too hard baked into the code?
--
Philip
^ permalink raw reply
* Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Junio C Hamano @ 2016-09-01 18:15 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Johannes Schindelin, git
In-Reply-To: <1472718808.4680.19.camel@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> +static const char *nth_for_number(int n)
>> +{
>> + int n1 = n % 10, n10 = n % 100;
>> +
>> + if (n1 == 1 && n10 != 11)
>> + return "st";
>> + if (n1 == 2 && n10 != 12)
>> + return "nd";
>> + if (n1 == 3 && n10 != 13)
>> + return "rd";
>> + return "th";
>> +}
>
>>8---
>
>> + if (command == TODO_SQUASH) {
>> + unlink(rebase_path_fixup_msg());
>> + strbuf_addf(&buf, "\n%c This is the %d%s commit message:\n\n%s",
>> + comment_line_char,
>> + count, nth_for_number(count), body);
>> + }
>> + else if (command == TODO_FIXUP) {
>> + strbuf_addf(&buf,
>> + "\n%c The %d%s commit message will be skipped:\n\n",
>> + comment_line_char, count, nth_for_number(count));
>> + strbuf_add_commented_lines(&buf, body, strlen(body));
>> + }
>
> This way of handling numbers is not translatable, and I really think we
> should mark these strings for translation, like they are in the .sh
> version.
Correct.
For those who were not paying attention on the 'master' front during
this pre-release period [*1*], I have to point out that the scripted
Porcelain has been updated to lose the Anglo-centric st/nd/rd/th and
this series would want to get updated to match.
[Footnote]
*1* Why weren't you? Repent! ;-)
^ permalink raw reply
* Re: [PATCH 21/22] sequencer: left-trim the lines read from the script
From: Junio C Hamano @ 2016-09-01 17:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narębski, git
In-Reply-To: <alpine.DEB.2.20.1609011608440.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > Interactive rebase's scripts may be indented; We need to handle this
>> > case, too, now that we prepare the sequencer to process interactive
>> > rebases.
>>
>> s/; We need/; we need/
>
> Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:
>
> There is some disagreement among writing reference manuals about
> when you should capitalize an independent clause following a
> colon. Most of the manuals advise that when you have more than one
> sentence in your explanation or when your sentence(s) is a formal
> quotation, a capital is a good idea. The NYPL Writer's Guide urges
> consistency within a document; the Chicago Manual of Style says
> you may begin an independent clause with a lowercase letter unless it's
> one of those two things (a quotation or more than one sentence).
> The APA Publication Manual is the most extreme: it advises us to
> always capitalize an independent clause following a colon. The advice
> given above is consistent with the Gregg Reference Manual.
>
> Based on that, I think that a capital is the correct case here.
Does that manual have anything to say about semicolons, which is a
different thing?
^ permalink raw reply
* Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-09-01 17:15 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Lars Schneider, git, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright
In-Reply-To: <20160830145429.GA11221@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
>>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index d5e1121..759991e 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -1485,7 +1485,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
>>
>> int git_open_noatime(const char *name)
>
> Hm, should the function then be renamed into
>
> git_open_noatime_cloexec()
>
>> {
>> - static int sha1_file_open_flag = O_NOATIME;
>> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
Perhaps.
In any case, this is probably something that can and should be done
outside this series.
I am tempted to suggest that the patch 13/13 under discussion may
also want to be done outside the scope of, and before, this series.
Even though with the current system an inherited file descriptor to
v1 filter processes would cause issues, there is no good reason to
expose this file desciptor to them.
Thanks.
^ permalink raw reply
* Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing
From: Junio C Hamano @ 2016-09-01 18:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Stefan Beller, Jakub Narębski, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1609010830110.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> git continue as a shorthand for `git <relevant-cmd> --continue` sounds great.
>
> Before we get ahead of ourselves:
>
> 1) this has nothing to do with the patch series at hand, and
>
> 2) if we were to introduce `git continue`, we would need to think long and
> hard about the following issues:
>
> I) are there potentially ambiguous <relevant-cmd>s that the user
> may want to continue?
>
> II) what about options? You can say `git rebase --continue
> --no-ff`, for example, but not `git cherry-pick --continue
> --no-ff`...
>
> III) Would it not be confusing to have a subcommand `continue`
> that does *not* serve a *single* purpose? It's kinda flying
> into the face of the Unix philosophy.
The above reasoning applies equally to "git abort". I do not think
"git continue" would help.
If it were that anything you can do with Git can be --continue'ed
the same way (e.g. all uses one sequencer to rule them all), it
might be achievable, but I do not think it isn't, and will never be.
"git commit" may say "You haven't added anything yet" and refuse to
do anything. Should "git continue" do "git commit -a" by noticing
that the last thing you tried to do was "git commit" and guess that
it is likely you wanted to commit all changes? I think not.
^ 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