* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Junio C Hamano @ 2016-12-05 22:43 UTC (permalink / raw)
To: Jeff King; +Cc: Jack Bates, git, Jack Bates
In-Reply-To: <20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I agree that it may be an accident waiting to happen, though, as soon as
> some buried sub-function needs to care about the distinction.
Yes to that.
>> I wonder if we're better off if we made sure that diff_no_index()
>> works the same way regardless of the value of "have_repository"
>> field?
>
> If you mean adding a diffopt flag like "just abbreviate everything to
> FALLBACK_DEFAULT_ABBREV even if we're in a repository", and then setting
> that in diff_no_index(), I agree that is a lot cleaner.
I am not sure if that is what I meant (I no longer sure what exactly
I meant to say there TBH), but this is probably not limited to the
default abbrev length aka core.abbrev configuration. Don't we have
other configuration settings we may read from $HOME/.gitconfig (and
possibly per-repository .git/config, if we did discovery but were
explicitly given "--no-index") that want to affect the behaviour of
the command?
I guess what I wanted, with "the same way", to see happen was that
"have_repository" should be only controling how and from what files
the configuration is read, and the behaviour of the command should
be controlled by the values read from the configuration after that.
Specifically, even if we were running with "--no-index", if we know
we have access to the current repository discovered by setting it up
gently, I do not think it is bad to ask find_unique_abbrev() to come
up with an appropriate abbreviation. So the fact that patch in
question has to flip the have_repository bit off, if it is done in
order to affect what diff_abbrev_oid() does, smells quite fishy from
that point of view.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 22:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <xmqqr35m2eva.fsf@gitster.mtv.corp.google.com>
On 05/12/16 22:24, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> As I said, the original version of the patch just removed the
>> --abbrev=7, but then I started to think about why you might have
>> used --abbrev in the first place (first in commit 9b88fcef7 and
>> again in commit bf505158d). Making sure to override the configuration
>> was the only thing I could come up with. So, I was hoping you could
>> remember why! :-P
>
> Nope. As a maintainer support script, the only thing I cared about
> it is that there is no -gXXXX at the end for anything I release ;-)
>
>> (I assumed it was to force a measure of uniformity/reproducibility).
>
> You cannot force uniformity/reproducibility with fixed abbrev,
> unless you set abbreviation length to 40, so you are correct to add
> "a measure of" there ;-)
Indeed. ;-)
> The first choice (i.e. 4) may have had a
> justification to force absolute minimum, and the second one (i.e. 7)
> may have had a justifiation to make it clear that we are using the
> same setting as the default, so in post-1.7.10 era, I think it is
> fine for us to just say "we have been using the same as default, so
> let's not specify anything explicitly".
So, you would be happy with just removing the '--abbrev' argument?
(That's fine by me; I don't set core.abbrev!)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Brandon Williams @ 2016-12-05 22:26 UTC (permalink / raw)
To: Stefan Beller
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <CAGZ79kaMXmNUaUCZJaTrcodAoSBPSxXr5JPrZGQeg=m-HrN11w@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> On Mon, Dec 5, 2016 at 11:29 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/02, Stefan Beller wrote:
> >> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
> >> + test_config checkout.recurseSubmodules 1 &&
> >> + git checkout base &&
> >> + git checkout -b advanced-base &&
> >> + git -C submodule commit --allow-empty -m "empty commit" &&
> >> + git add submodule &&
> >> + git commit -m "advance submodule" &&
> >> + git checkout base &&
> >> + git diff-files --quiet &&
> >> + git diff-index --quiet --cached base &&
> >> + git checkout advanced-base &&
> >> + git diff-files --quiet &&
> >> + git diff-index --quiet --cached advanced-base &&
> >> + git checkout --recurse-submodules base
> >> +'
> >> +
> >
> > This test doesn't look like it looks into the submodule to see if the
> > submodule has indeed changed. Unless diff-index and diff-files recurse
> > into the submodules?
>
> I took the code from Jens once upon a time. Rereading the code, I agree it is
> not obvious how this checks the submodule state.
>
> However `git diff-files --quiet` is perfectly fine, as
> we have submodule support by default via:
>
> Omitting the --submodule option or specifying --submodule=short,
> uses the short format. This format just shows the names of the commits
> at the beginning and end of the range.
>
> and then we turn it into an exit code via
>
> --quiet
> Disable all output of the program. Implies --exit-code.
>
> --exit-code
> Make the program exit with codes similar to diff(1).
> That is, it exits with 1 if there were differences and 0 means no differences.
>
> Same for diff-index.
>
> The main purpose of this specific test is to have checkout.recurseSubmodules
> "to just make it work" without having to give --recurse-submodules manually.
> All the other tests with the manual --recurse-submodules should test for
> correctness of the behavior within the submodule.
>
> So maybe I'll need to rewrite submodule_creation_must_succeed() in the previous
> patch to be more obvious. (Well that already has some tests for
> files/directories
> in there, so it is a little more.)
>
> But to be sure we can also add tests here that look more into the submodule.
> I am thinking of "{new,old}_sub_sha1=$(git -C submodule rev-parse HEAD)" and
> comparing them?
Ah ok, that makes sense now. Its kind of like if I run git status it
would show if a submodule is at a different sha1 than the superproject
has recorded.
--
Brandon Williams
^ permalink raw reply
* [PATCH] difftool: fix dir-diff index creation when in a subdirectory
From: David Aguilar @ 2016-12-05 22:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML, Frank Becker, Johannes Schindelin, John Keeping
9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
are handled in a subdirectory, but it introduced a regression in how
entries outside of the subdirectory are handled by dir-diff.
When preparing the right-side of the diff we only include the
changed paths in the temporary area.
The left side of the diff is constructed from a temporary
index that is built from the same set of changed files, but it
was being constructed from within the subdirectory. This is a
problem because the indexed paths are toplevel-relative, and
thus they were not getting added to the index.
Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes. This ensures that all of the
toplevel-relative paths are valid.
Adjust the test cases to more thoroughly exercise this scenario.
Reported-by: Frank Becker <fb@mooflu.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
I figured I'd send this as a proper patch.
The patch contents have not changed since the original in-thread
patch, but the commit message was modified slightly.
git-difftool.perl | 4 ++++
t/t7800-difftool.sh | 7 +++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..959822d5f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ EOF
}
}
+ # Go to the root of the worktree so that the left index files
+ # are properly setup -- the index is toplevel-relative.
+ chdir($workdir);
+
# Setup temp directories
my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461..caab4b5ca 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -413,8 +413,11 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
- grep sub output &&
- grep file output
+ # "sub" must only exist in "right"
+ # "file" and "file2" must be listed in both "left" and "right"
+ test "1" = $(grep sub output | wc -l) &&
+ test "2" = $(grep file"$" output | wc -l) &&
+ test "2" = $(grep file2 output | wc -l)
)
'
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-05 22:24 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <e74163ee-e7d4-bcbd-e65f-368bc2ee9a2d@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> As I said, the original version of the patch just removed the
> --abbrev=7, but then I started to think about why you might have
> used --abbrev in the first place (first in commit 9b88fcef7 and
> again in commit bf505158d). Making sure to override the configuration
> was the only thing I could come up with. So, I was hoping you could
> remember why! :-P
Nope. As a maintainer support script, the only thing I cared about
it is that there is no -gXXXX at the end for anything I release ;-)
> (I assumed it was to force a measure of uniformity/reproducibility).
You cannot force uniformity/reproducibility with fixed abbrev,
unless you set abbreviation length to 40, so you are correct to add
"a measure of" there ;-) The first choice (i.e. 4) may have had a
justification to force absolute minimum, and the second one (i.e. 7)
may have had a justifiation to make it clear that we are using the
same setting as the default, so in post-1.7.10 era, I think it is
fine for us to just say "we have been using the same as default, so
let's not specify anything explicitly".
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Stefan Beller @ 2016-12-05 22:23 UTC (permalink / raw)
To: Brandon Williams
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <20161205192918.GB68588@google.com>
On Mon, Dec 5, 2016 at 11:29 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/02, Stefan Beller wrote:
>> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
>> + test_config checkout.recurseSubmodules 1 &&
>> + git checkout base &&
>> + git checkout -b advanced-base &&
>> + git -C submodule commit --allow-empty -m "empty commit" &&
>> + git add submodule &&
>> + git commit -m "advance submodule" &&
>> + git checkout base &&
>> + git diff-files --quiet &&
>> + git diff-index --quiet --cached base &&
>> + git checkout advanced-base &&
>> + git diff-files --quiet &&
>> + git diff-index --quiet --cached advanced-base &&
>> + git checkout --recurse-submodules base
>> +'
>> +
>
> This test doesn't look like it looks into the submodule to see if the
> submodule has indeed changed. Unless diff-index and diff-files recurse
> into the submodules?
I took the code from Jens once upon a time. Rereading the code, I agree it is
not obvious how this checks the submodule state.
However `git diff-files --quiet` is perfectly fine, as
we have submodule support by default via:
Omitting the --submodule option or specifying --submodule=short,
uses the short format. This format just shows the names of the commits
at the beginning and end of the range.
and then we turn it into an exit code via
--quiet
Disable all output of the program. Implies --exit-code.
--exit-code
Make the program exit with codes similar to diff(1).
That is, it exits with 1 if there were differences and 0 means no differences.
Same for diff-index.
The main purpose of this specific test is to have checkout.recurseSubmodules
"to just make it work" without having to give --recurse-submodules manually.
All the other tests with the manual --recurse-submodules should test for
correctness of the behavior within the submodule.
So maybe I'll need to rewrite submodule_creation_must_succeed() in the previous
patch to be more obvious. (Well that already has some tests for
files/directories
in there, so it is a little more.)
But to be sure we can also add tests here that look more into the submodule.
I am thinking of "{new,old}_sub_sha1=$(git -C submodule rev-parse HEAD)" and
comparing them?
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-05 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqr35m3zx7.fsf@gitster.mtv.corp.google.com>
On 12/05, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > On 12/01, Junio C Hamano wrote:
> >> Brandon Williams <bmwill@google.com> writes:
> >>
> >> > I started taking a look at your http redirect series (I really should
> >> > have taking a look at it sooner) and I see exactly what you're talking
> >> > about. We can easily move this logic into a function to make it easier
> >> > to generate the two whitelists.
> >>
> >> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
> >> expect that a future reroll would make things better.
> >
> > Yeah I expected we would see an issue since both these series collide in
> > http.c
> >
> > I'm sending out another reroll of this series so that in Jeff's he can
> > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > option, which should make this test stop barfing.
>
> I was hoping to eventually merge Peff's series to older maintenance
> tracks. How bad would it be if we rebased the v8 of this series
> together with Peff's series to say v2.9 (or even older if it does
> not look too bad)?
I just took Jeff's series and applied it on top of mine (and fixed the
small problem causing t5812 to fail) and then rebased it to v2.9.0.
There were a few issues that needed to be resolved but nothing too
hairy. So it would definitely be doable to backport it to the
maintenance tracks.
--
Brandon Williams
^ permalink raw reply
* Re: git repo vs project level authorization
From: Junio C Hamano @ 2016-12-05 21:59 UTC (permalink / raw)
To: ken edward; +Cc: git
In-Reply-To: <CAAqgmoO+7cLZHpX61=Mh7PjqrCUc0qyFD=C+sjVat_+KPhisbw@mail.gmail.com>
ken edward <kedward777@gmail.com> writes:
> I am currently using svn with apache+mod_dav_svn to have a single
> repository with multiple projects. Each of the projects is controlled
> by an access control file that lists the project path and the allowed
> usernames.
>
> Does git have this also? where is the doc?
It is customary not to mix unrelated projects into a single
repository in the Git land. Some hosting solutions give access
control per branches, and some others give access control per
repositories.
^ permalink raw reply
* Re: git repo vs project level authorization
From: Fredrik Gustafsson @ 2016-12-05 22:04 UTC (permalink / raw)
To: ken edward; +Cc: git
In-Reply-To: <CAAqgmoO+7cLZHpX61=Mh7PjqrCUc0qyFD=C+sjVat_+KPhisbw@mail.gmail.com>
On Mon, Dec 05, 2016 at 03:33:51PM -0500, ken edward wrote:
> I am currently using svn with apache+mod_dav_svn to have a single
> repository with multiple projects. Each of the projects is controlled
> by an access control file that lists the project path and the allowed
> usernames.
>
> Does git have this also? where is the doc?
>
> Ken
Git does not do hosting or access control. For this you need to use a
third party program. There are plenty of options for you and each has
different features and limitations. For example you should take a look
at gitolite, gitlab, bitbucket, github, gogs. Just to mention a few.
It's also possible to setup git with ssh or http/https with your own
access control methods. See the progit book for details here.
--
Fredrik Gustafsson
phone: +46 733-608274
e-mail: iveqy@iveqy.com
website: http://www.iveqy.com
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Luke Diamand @ 2016-12-05 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com>
On 5 December 2016 at 21:24, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>>> Luke Diamand <luke@diamand.org> writes:
>>>
>>>> Teach git-p4 about git-workspaces.
>>>
>>> Is this what we call "git worktree", or something else?
>>
>> Ah, I think you're right!
>
> Then I'll queue it like the attached.
>
> HOWEVER.
>
> How fast does isValidGitDir() function need to be?
It doesn't need to be fast.
> The primary one
> seems to check HEAD (but it does not notice a non-repository that
> has a directory with that name), refs and objects (but it does not
> notice a non-repository that has a non-directory with these names),
> and this new one uses a test that is even more sloppy.
>
> What I am trying to get at is if we want to use a single command
> that can be given a path and answer "Yes, that is a repository"
> here, and that single command should know how the repository should
> look like. I offhand do not know we already have such a command we
> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
> perhaps we would want one, so that not just "git p4" but other
> scripted Porcelains can make use of it?
That would be nicer than random ad-hoc rules, certainly. I couldn't
find any obvious combination that would work.
Luke
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 21:54 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Luke Diamand <luke@diamand.org> writes:
> ...
> if (os.path.exists(path + "/HEAD")
> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> return True;
> +
> + # secondary working tree managed by "git worktree"?
> + if (os.path.exists(path + "/HEAD")
> + and os.path.exists(path + "/gitdir")):
> + return True
> +
> return False
Wait a bit. How is this expected to work?
I assume "path" is something like "$somewhere/.git" (either
absolute or relative), and by concatenating HEAD, refs and objects
under it the original checks form paths to expected directories,
because "$somewhere/.git" is a directory.
A worktree created with "git worktree add" gets ".git" that is a
file, and the contents of the file records the path to a
subdirectory "worktrees/$name" of the primary repository the
worktree is borrowing from, i.e. "$somewhere/.git/worktrees/$name".
So for this patch to work correctly, the caller, when in a "git
worktree" managed secondary working tree, MUST have found the ".git"
file at the root level of the working tree, MUST have read its
contents to learn that "$somewhere/.git/worktrees/$name" path, and
then feeding it to this function. But doesn't such a caller already
know that it is a valid repository? After all, it trusted the
contents of that ".git" file at the root level.
... goes and looks at the callers ...
Between the two call sites of isValidGitDir() in main(), the first
one (i.e. if ".git" in the current directory, made into abspath,
does not look like a repository, ask "rev-parse --git-dir" for one)
looks correct and I think it would grab the correct $GIT_DIR just
fine [*1*]. Isn't the real problem the second one, i.e. the one
that feeds a correct cmd.gitdir that we just obtained by asking
"rev-parse --git-dir" to the sloppy isValidGitDir() again. The
latter second guesses "rev-parse --git-dir" and botches.
I have a feeling that adding more to isValidGitDir() like this patch
does is a step in the wrong direction. The first fix would be not
to do the "if isValidGitDir() says no, then do something else" step
if you already asked "rev-parse --git-dir" and got a valid $GIT_DIR,
perhaps?
Stepping back even more.
I wonder why the script needs to do os.environ.get("GIT_DIR") in the
first place to initialize cmd.gitdir field. If it instead asked
"rev-parse --gitdir", the script would get the right answer that
already takes GIT_DIR environment into account.
[Footnote]
*1* This ends up asking "rev-parse --git-dir" only because
isValidGitDir() is sloppy. If you are in a secondary working
tree whose ".git" is a file, the function would say "no", and
that is the only thing that allows you to make a call to
"rev-parse --git-dir" to obtain the correct result.
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 21:24 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <CAE5ih78Y_AbfgtW_6zMKLC8NzBxCKSagrgrjtfWZVOEwaAg6ZA@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> Teach git-p4 about git-workspaces.
>>
>> Is this what we call "git worktree", or something else?
>
> Ah, I think you're right!
Then I'll queue it like the attached.
HOWEVER.
How fast does isValidGitDir() function need to be? The primary one
seems to check HEAD (but it does not notice a non-repository that
has a directory with that name), refs and objects (but it does not
notice a non-repository that has a non-directory with these names),
and this new one uses a test that is even more sloppy.
What I am trying to get at is if we want to use a single command
that can be given a path and answer "Yes, that is a repository"
here, and that single command should know how the repository should
look like. I offhand do not know we already have such a command we
can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
perhaps we would want one, so that not just "git p4" but other
scripted Porcelains can make use of it?
-- >8 --
From: Luke Diamand <luke@diamand.org>
Date: Fri, 2 Dec 2016 22:43:18 +0000
Subject: [PATCH] git-p4: support secondary working trees managed by "git worktree"
Teach git-p4 about them.
Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-p4.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..b3c50ae7e5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
if (os.path.exists(path + "/HEAD")
and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
return True;
+
+ # secondary working tree managed by "git worktree"?
+ if (os.path.exists(path + "/HEAD")
+ and os.path.exists(path + "/gitdir")):
+ return True
+
return False
def parseRevision(ref):
--
2.11.0-222-g22b1346184
^ permalink raw reply related
* Re: [PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default
From: Ori Rawlings @ 2016-12-05 21:19 UTC (permalink / raw)
To: Git Users
In-Reply-To: <20161204140311.26269-1-larsxschneider@gmail.com>
Looks good to me, too.
-r flag seems to be supported as far back as I can search in the Helix
release notes.
Ori Rawlings
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Luke Diamand @ 2016-12-05 21:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq8tru3xom.fsf@gitster.mtv.corp.google.com>
On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> Teach git-p4 about git-workspaces.
>
> Is this what we call "git worktree", or something else?
Ah, I think you're right!
>
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>> git-p4.py | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 0c4f2afd2..5e2db1919 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -566,6 +566,12 @@ def isValidGitDir(path):
>> if (os.path.exists(path + "/HEAD")
>> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
>> return True;
>> +
>> + # git workspace directory?
>> + if (os.path.exists(path + "/HEAD")
>> + and os.path.exists(path + "/gitdir")):
>> + return True
>> +
>> return False
>>
>> def parseRevision(ref):
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 20:53 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Vinicius Kursancew, larsxschneider
In-Reply-To: <20161202224319.5385-2-luke@diamand.org>
Luke Diamand <luke@diamand.org> writes:
> Teach git-p4 about git-workspaces.
Is this what we call "git worktree", or something else?
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> git-p4.py | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0c4f2afd2..5e2db1919 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -566,6 +566,12 @@ def isValidGitDir(path):
> if (os.path.exists(path + "/HEAD")
> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> return True;
> +
> + # git workspace directory?
> + if (os.path.exists(path + "/HEAD")
> + and os.path.exists(path + "/gitdir")):
> + return True
> +
> return False
>
> def parseRevision(ref):
^ permalink raw reply
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Junio C Hamano @ 2016-12-05 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Krey, git
In-Reply-To: <20161203043254.7ozjyucfn6uivnsh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:
>
>> --only is implied when paths are present, and required
>> them unless --amend. But with --allow-empty it should
>> be allowed as well - it is the only way to create an
>> empty commit in the presence of staged changes.
>
> OK. I'm not sure why you would want to create an empty commit in such a
> case. But I do agree that this seems like a natural outcome for "--only
> --allow-empty". So whether it is particularly useful or not, it seems
> like the right thing to do. The patch itself looks good to me.
Slightly related topic.
>> - if (argc == 0 && (also || (only && !amend)))
>> + if (argc == 0 && (also || (only && !amend && !allow_empty)))
>> die(_("No paths with --include/--only does not make sense."));
>> if (argc == 0 && only && amend)
>> only_include_assumed = _("Clever... amending the last one with dirty index.");
>
We allow "-o --amend" without no pathspec because that is how you
would reword without changing the tree object in the tip commit, and
we reward the user who figured out such an esoteric use with a
message "Clever...". I do not think if people who say "I want to
create an empty commit but I already have added changes to the
index" deserve the same "Clever..." praise, so I will not suggest
adding another message above.
More seriously, I suspect that the message outlived its usefulness.
If we wanted to make the "use --amend -o without pathspec if you
want to reword the tip one without touching its tree" easier to
discover, the place to do so is in the documentation, not a message
that is given as a reward to those who already discovered it.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 20:38 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <20161205201237.GD68588@google.com>
On Mon, Dec 5, 2016 at 12:12 PM, Brandon Williams <bmwill@google.com> wrote:
>> > + if (path->len > 1) {
>> > + char *last_slash = find_last_dir_sep(path->buf);
>>
>> What happens when there is no dir_sep?
>
> There should always be a dir_sep since that only gets run if the passed
> in path at least contains root '/'
Oh, sure, that makes sense. When porting/running this on Windows, does
the assumption still hold?
>> if (strbuf_getcwd(&sb))
>> die_errno(_("unable to get current working directory"));
>>
>> Not sure if aligning them would be a good idea?
>>
>> Going by "git grep die_errno" as well as our Coding guidelines,
>> we don't want to see capitalized error messages.
>
> K I can use the other msg.
Well this wasn't a rhetorical question, but I was genuine wondering
if that was worth it.
When having different error messages in different places,
it makes debugging easier, because you have fewer starting points.
But this function is deep down in the stack, such that you would expect
other error messages to also show up , so I dunno.
>> > + } else if (S_ISLNK(st.st_mode)) {
>>
>> As far as I can tell, we could keep the symlink strbuf
>> at a smaller scope here? (I was surprised how many strbufs
>> are declared at the beginning of the function)
>
> Yeah I can push it down in scope. There will be a bit more allocation
> churn with the smaller scope but multiple symlinks should be rare?
> Alternatively the 'next' buffer can be reused...I decided against that
> initially due to readability.
I'd second to not reuse 'next'. :)
I guess we could keep the less churn-y version then.
> And yes, lots of string manipulation
> requires lots of strbufs :)
^ permalink raw reply
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Junio C Hamano @ 2016-12-05 20:36 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Krey, git
In-Reply-To: <20161203162318.uv27n4uhylobegto@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
>
>> > OK. I'm not sure why you would want to create an empty commit in such a
>> > case.
>>
>> User: Ok tool, make me a pullreq.
>>
>> Tool: But you haven't mentioned any issue
>> in your commit messages. Which are they?
>>
>> User: Ok, that would be A-123.
>>
>> Tool: git commit --allow-empty -m 'FIX: A-123'
>
> OK. I think "tool" is slightly funny here, but I get that is part of the
> real world works. Thanks for illustrating.
I am not sure if I understand. Why isn't the FIX: thing added to
the commit being pulled by amending it? Would the convention be for
the responder of a pull-request to fetch and drop the tip commit?
^ permalink raw reply
* git repo vs project level authorization
From: ken edward @ 2016-12-05 20:33 UTC (permalink / raw)
To: git
I am currently using svn with apache+mod_dav_svn to have a single
repository with multiple projects. Each of the projects is controlled
by an access control file that lists the project path and the allowed
usernames.
Does git have this also? where is the doc?
Ken
^ permalink raw reply
* Re: [PATCH v4 1/3] update-unicode.sh: automatically download newer definition files
From: Junio C Hamano @ 2016-12-05 20:31 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Beat Bolli, git
In-Reply-To: <20161204075800.GA2415@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
> On Sat, Dec 03, 2016 at 10:00:47PM +0100, Beat Bolli wrote:
>> Checking just for the unicode data files' existence is not sufficient;
>> we should also download them if a newer version exists on the Unicode
>> consortium's servers. Option -N of wget does this nicely for us.
>>
>> Reviewed-by: Torsten Boegershausen <tboegi@web.de>
>
> Minor remark (Not sure if this motivates v5, may be Junio can fix it locally?)
> s/oe/ö/
>
> Beside this: Thanks again (and I learned about the -N option of wget)
Will fix up while queuing (only 1/3 needs it, 2/3 has it right).
Also, I'll do s/update-unicode.sh/update_unicode.sh/ on the title
and the message to match the reality. At some point we might want
to fix the reality to match people's expectations, though.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 20:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <xmqq7f7e5jsy.fsf@gitster.mtv.corp.google.com>
On 05/12/16 18:10, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Heh, that was the first version of the patch. However, I got to thinking
>> about why --abbrev=7 was there in the first place; the only reason I
>> could think of was to defeat local configuration to get a measure of
>> reproducibility.
>>
>> Unfortunately, you can't get the 'auto' behaviour from --abbrev
>> (on the pu branch):
>>
>> $ ./git describe --abbrev=-1
>> v2.11.0-286-g109e8
>> $ ./git describe --abbrev=0
>> v2.11.0
>> $ ./git describe
>> v2.11.0-286-g109e8a99d
>> $
>
> What is the reason why the last one is undesirable? Is it because
> the user may have core.abbrev set to some value in the configuration
> and you want to override it to force "auto"?
As I said, the original version of the patch just removed the
--abbrev=7, but then I started to think about why you might have
used --abbrev in the first place (first in commit 9b88fcef7 and
again in commit bf505158d). Making sure to override the configuration
was the only thing I could come up with. So, I was hoping you could
remember why! :-P
(I assumed it was to force a measure of uniformity/reproducibility).
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 20:12 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CAGZ79kauPdE1uiFSvBALkNiwXbnV6d6xhwLdWNQwRir_8rTG6Q@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> > +/* removes the last path component from 'path' except if 'path' is root */
> > +static void strip_last_component(struct strbuf *path)
> > +{
> > + if (path->len > 1) {
> > + char *last_slash = find_last_dir_sep(path->buf);
>
> What happens when there is no dir_sep?
There should always be a dir_sep since that only gets run if the passed
in path at least contains root '/'
>
> > +/* gets the next component in 'remaining' and places it in 'next' */
> > +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> > +{
>
> It's more than just getting it, it also chops it off of 'remaining' ?
True, I can update the comment to reflect that.
> > + } else {
> > + /* relative path; can use CWD as the initial resolved path */
> > + if (strbuf_getcwd(&resolved)) {
> > + if (die_on_error)
> > + die_errno("Could not get current working directory");
>
> I am looking at xgetcwd, which words it slightly differently.
>
> if (strbuf_getcwd(&sb))
> die_errno(_("unable to get current working directory"));
>
> Not sure if aligning them would be a good idea?
>
> Going by "git grep die_errno" as well as our Coding guidelines,
> we don't want to see capitalized error messages.
K I can use the other msg.
> >
> > - if (sb.len) {
> > - if (!cwd.len && strbuf_getcwd(&cwd)) {
> > + /* append the next component and resolve resultant path */
>
> "resultant" indicates you have a math background. :)
> But I had to look it up, I guess it is fine that way,
> though "resulting" may cause less mental friction
> for non native speakers.
>
>
> > + if (!(errno == ENOENT && !remaining.len)) {
> > if (die_on_error)
> > - die_errno("Could not get current working directory");
> > + die_errno("Invalid path '%s'",
> > + resolved.buf);
> > else
> > goto error_out;
> > }
> > + } else if (S_ISLNK(st.st_mode)) {
>
> As far as I can tell, we could keep the symlink strbuf
> at a smaller scope here? (I was surprised how many strbufs
> are declared at the beginning of the function)
Yeah I can push it down in scope. There will be a bit more allocation
churn with the smaller scope but multiple symlinks should be rare?
Alternatively the 'next' buffer can be reused...I decided against that
initially due to readability. And yes, lots of string manipulation
requires lots of strbufs :)
> > + //strbuf_release(&resolved);
>
> This is why the cover letter toned down expectations ?
> (no // as comment, maybe remove that line?)
yep. It will be added back in though once the callers to real_path take
ownership of the memory.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 20:16 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CAGZ79katWewdwU3ZDYDj-QZEeersx9XDPZuTdMJG_u_62YwMsg@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> > static const char *real_path_internal(const char *path, int die_on_error)
> > {
> > - static struct strbuf sb = STRBUF_INIT;
> > + static struct strbuf resolved = STRBUF_INIT;
>
> Also by having this static here, it is not quite thread safe, yet.
>
> By removing the static here we cannot do the early cheap check as:
>
> > /* We've already done it */
> > - if (path == sb.buf)
> > + if (path == resolved.buf)
> > return path;
>
> I wonder how often we run into this case; are there some callers explicitly
> relying on real_path_internal being cheap for repeated calls?
> (Maybe run the test suite with this early return instrumented? Not sure how
> to assess the impact of removing the cheap out return optimization)
>
> The long tail (i.e. the actual functionality) should actually be
> faster, I'd imagine
> as we do less than with using chdir.
Depends on how expensive the chdir calls were. And I'm working to get
rid of the static buffer. Just need have the callers own the memory
first.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 20:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
> static const char *real_path_internal(const char *path, int die_on_error)
> {
> - static struct strbuf sb = STRBUF_INIT;
> + static struct strbuf resolved = STRBUF_INIT;
Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
> /* We've already done it */
> - if (path == sb.buf)
> + if (path == resolved.buf)
> return path;
I wonder how often we run into this case; are there some callers explicitly
relying on real_path_internal being cheap for repeated calls?
(Maybe run the test suite with this early return instrumented? Not sure how
to assess the impact of removing the cheap out return optimization)
The long tail (i.e. the actual functionality) should actually be
faster, I'd imagine
as we do less than with using chdir.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-05 20:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161201235856.GL54082@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 12/01, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > I started taking a look at your http redirect series (I really should
>> > have taking a look at it sooner) and I see exactly what you're talking
>> > about. We can easily move this logic into a function to make it easier
>> > to generate the two whitelists.
>>
>> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
>> expect that a future reroll would make things better.
>
> Yeah I expected we would see an issue since both these series collide in
> http.c
>
> I'm sending out another reroll of this series so that in Jeff's he can
> just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> option, which should make this test stop barfing.
I was hoping to eventually merge Peff's series to older maintenance
tracks. How bad would it be if we rebased the v8 of this series
together with Peff's series to say v2.9 (or even older if it does
not look too bad)?
^ permalink raw reply
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