* Re: [Bug] Custom git-dir directory shouldn't be listed as “untracked”
From: Junio C Hamano @ 2016-09-19 17:45 UTC (permalink / raw)
To: Nicolas Cuillery; +Cc: git
In-Reply-To: <CACmQg1hE=ytaatDfUJLkhL0p5c43wZZvgt+8pc5zoo0YFdQw6A@mail.gmail.com>
Nicolas Cuillery <nicolas.cuillery@gmail.com> writes:
> When using the default directory ".git", it logically doesn't appear
> in the "git status" command's output. Don't you think it should be the
> same when using a custom dir name ?
Not really.
GIT_DIR=<there> mechanism was never meant to be used to name a
directory that sitsinside your working tree (an exception is if it
is actually ".git" at the top).
^ permalink raw reply
* Re: [PATCH v3 0/8] Better heuristics make prettier diffs
From: Junio C Hamano @ 2016-09-19 17:27 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Ramsay Jones, René Scharfe, Stefan Beller, Jeff King,
Jakub Narębski, Jacob Keller
In-Reply-To: <xmqqd1jzho2l.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> On 09/08/2016 01:25 AM, Junio C Hamano wrote:
>>> I'd move it temporarily to t4061 with a separate SQUASH??? at the
>>> tip for now, as I am running out of time today.
>>
>> I didn't realize you were waiting for an ACK. Yes, it's totally OK to
>> rename the test.
>
> I actually wasn't asking for an Ack.
>
> As the issue was in the one that is buried a few commits from the
> tip, and there is a later one that adds more tests to it, I didn't
> find enough energy to rename the new file in a buried commit and
> then adjust the patch later updates it, I was hoping that you'd
> reroll to save me effort, rather than forcing me to do the rebase
> myself ;-).
Now I did, so no need to resend (unless you have changes other than
the renaming of the test script, that is).
Let's move it down to 'next' soonish.
Thanks.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-19 17:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <xmqqvaxrg6zt.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 19, 2016 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I think you were clear enough.
>
> Don't read everything other people say in their reviews as pointing
> out issues. Often trying to rephrase what they read in the code in
> their own words is a good way to make sure the reviewers and the
> original author are on the same page. The above was one of these
> cases.
That makes sense, I'll be sure to remember that!
>
>>>> + if (prefix > 0) {
>>>> + if (ps_strncmp(item, pattern, string, prefix))
>>>> + return WM_NOMATCH;
>>>
>>> This says: when we have a set prefix that must literally match, and
>>> that part does not match what we have, it cannot possibly match.
>>>
>>> Is that correct? What do we have in "name" and "item" at this
>>> point? We disable the common-prefix optimization, so we do not have
>>> to worry about a pathspec with two elements "sub/dir1/*" and "sub/dir2/*"
>>> giving you "sub/dir" as the common prefix, when you are wondering if
>>> it is worth descending into "sub/" without knowing what it contains.
>>> Is that what guarantees why this part is correct?
>>
>> I adopted this structure from another part of the code. The caller
>> uses a field in
>> the pathspec item which indicates the location of the first wildcard character.
>> So the prefix (everything prior to the wildcard char) must match
>> literally before
>> we drop into a more expensive wildmatch function.
>
> "Another part of the code" is about tree walking, right? Weren't
> you saying that part of the code may be buggy or something earlier
> (e.g. pathspec "su?/" vs entry "sub")?
I was refering to the logic that is used to do normal pathspec checking:
'match_pathspec' and the functions it calls, in particular
git_fnmatch. And the bug I
pointed out before, I believe is due to how the wildmatch function
works. It requires
that the pattern and the string being compared must match exactly (in
length as well).
The "su?/" case would drop into wildmatch funciton and wouldn't match
against any file
in the directory "sub/file1" for example, because it doesn't exactly
match "su?/". In the
case of "sub" there is logic prior to the wildmatch function call
which would classify it a
match and return before descending into wildmatch.
> Again, what do we have in "name" and "item" at this point? If we
> have a submodule at "sub/" and we are checking a pathspec element
> "sub/dir1/*", what is the non-wildcard part of the pathspec and what
> is the "string"? Aren't then "sub/dir1/" and "sub/" respectively,
> which would not pass ps_strncmp() and produce a (false) negative?
item will be the pathspec_item struct that we are trying to match against.
name will be the file we are trying to match, which should already have the
'prefix' cut off (this is the prefix that is used as an optimization
in the common
case, which isn't used in the submodule case). The 'prefix' in this function's
context is the part of the pattern prior to the first wildcard character. which
we can do a literal comparison on before descending into the wildmatch function.
> I am starting to have a feeling that the best we can do in this
> function safely is to see if prefix (i.e. the constant part of the
> pathspec before the first wildcard) is long enough to cover the
> "name" and if "name" part does not match to the directory boundary,
> e.g. for this combination
>
> pathspec = "a/b/sib/c/*"
> name = "a/b/sub/"
>
> we can say with confidence that it is not worth descending into.
>
> When prefix is long enough and "name" and leading part of the prefix
> matches to the directory boundary, e.g.
>
> pathspec = "a/b/sub/c/*"
> name = "a/b/sub/"
>
> we can say it is worth descending into.
>
> If these two checks cannot decide, we may have to be pessimistic and
> say "it may match; we don't know until we descend into it". When
> prefix is shorter than name, I am not sure if we can devise a set of
> simple rules, e.g.
>
> pathspec = "a/**/c/*"
> name = "a/b/sub/"
>
> may match with its ** "b/sub" part and worth descending into, so is
>
> pathspec = "a/b/*/c/*"
> name = "a/b/sub/"
>
> but not this one:
>
> pathspec = "a/b/su[c-z]/c/*"
> name = "a/b/sub/"
>
> but this is OK:
>
> pathspec = "a/b/su[a-z]/c/*"
> name = "a/b/sub/"
>
> So I would think we'd be in the business of counting slashes in the
> name (called "string" in this function) and the pathspec, while
> noticing '*' and '**' in the latter, and we may be able to be more
> precise, but I am not sure how complex the end result would become.
>
I agree, I'm not too sure how much more complex the logic would need
to be to handle
all matters of wildcard characters. We could initially be more
lenient on what qualifies as
a match and then later (or in the near future) revisit the wildmatch
function (which is complex)
and see if we can add better matching capabilities more suited for
submodules while at the
same time fixing that bug discussed above.
^ permalink raw reply
* Re: [PATCH 11/11] Resumable clone: implement primer logic in git-clone
From: Junio C Hamano @ 2016-09-19 17:16 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Kevin Wern, Git Mailing List
In-Reply-To: <CACsJy8B1bbKBhg1ke4u6PV3k4FWz-bhBPyN2X=mV2Z2=8Mhy=A@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Sep 16, 2016 at 7:12 AM, Kevin Wern <kevin.m.wern@gmail.com> wrote:
>> builtin/clone.c | 590 +++++++++++++++++++++++++++++++++++++-------
>
> Argh.. this is too big for my brain at this hour. It might be easier
> to follow if you separate out some code move (I think I've seen some,
> not sure). I'll try to have another look when I find time. But it's
> great to hear from you again, the pleasant surprise in my inbox today,
> as I thought we lost you ;-) There's hope for resumable clone maybe
> before 2018 again.
I had a similar thought.
What "git clone" (WITHOUT Kevin's update) should have been was to be
as close to
* Parse command line arguments;
* Create a new repository and go into it; this step would
require us to have parsed the command line for --template,
<directory>, --separate-git-dir, etc.
* Talk to the remote and do get_remote_heads() aka ls-remote
output;
* Decide what fetch refspec to use, which alternate object store
to borrow from; this step would require us to have parsed the
command line for --reference, --mirror, --origin, etc;
* Issue "git fetch" with the refspec determined above; this step
would require us to have parsed the command line for --depth, etc.
* Run "git checkout -b" to create an initial checkout; this step
would require us to have parsed the command line for --branch,
etc.
and the current code Kevin is basing his work on is not quite in
that shape. This round of the patches may be RFC so it may be OK
but the ready-for-review work may need to do the refactoring to get
it close to the above shape as a preparatory step before doing
anything else. Once that is done, Kevin's series (other than the
part that acutally does the resumable static file download) will
become a very easily understood "Ah, we know where to prime the well
from, so let's do that first" step inserted immediately before the
"Issue 'git fetch'" step.
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-19 17:03 UTC (permalink / raw)
To: Ben Peart; +Cc: pclouds, 'Ben Peart', git
In-Reply-To: <xmqqzin3g8di.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> "git checkout -b foo" (without -f -m or <start_point>) is defined in the
>> manual as being a shortcut for/equivalent to:
>>
>> (1a) "git branch foo"
>> (1b) "git checkout foo"
>>
>> However, it has been our experience in our observed use cases and all the
>> existing git tests, that it can be treated as equivalent to:
>>
>> (2a) "git branch foo"
>> (2b) "git symbolic-ref HEAD refs/heads/foo"
>> ...
>
> I am still not sure if I like the change of what "checkout -b" is
> this late in the game, though.
Having said all that.
I do see the merit of having a shorthand way to invoke your 2 above.
It is just that I am not convinced that it is the best way to
achieve that goal to redefine what "git checkout -b <new-name>" (no
other parameters) does.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-19 17:00 UTC (permalink / raw)
To: Brandon Williams
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <CAKoko1r6cfv-2HVCJPgGbXyCVe-wdUBS+2nXtaTHO3jshVg8MA@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
>> OK, so as discussed previously with Heiko and Stefan, the idea is to
>>
>> - pass the original pathspec as-is,
>>
>> - when --submodule-prefix is given, a path discovered in a
>> submodule repository is first prefixed with that string before
>> getting checked to see if it matches the original pathspec.
>>
>> And this loop is about relaying the original pathspec.
>
> Exactly. Perhaps I should have made this more clear either with a
> detailed comment or
> more information in the commit msg.
I think you were clear enough.
Don't read everything other people say in their reviews as pointing
out issues. Often trying to rephrase what they read in the code in
their own words is a good way to make sure the reviewers and the
original author are on the same page. The above was one of these
cases.
>>> + if (prefix > 0) {
>>> + if (ps_strncmp(item, pattern, string, prefix))
>>> + return WM_NOMATCH;
>>
>> This says: when we have a set prefix that must literally match, and
>> that part does not match what we have, it cannot possibly match.
>>
>> Is that correct? What do we have in "name" and "item" at this
>> point? We disable the common-prefix optimization, so we do not have
>> to worry about a pathspec with two elements "sub/dir1/*" and "sub/dir2/*"
>> giving you "sub/dir" as the common prefix, when you are wondering if
>> it is worth descending into "sub/" without knowing what it contains.
>> Is that what guarantees why this part is correct?
>
> I adopted this structure from another part of the code. The caller
> uses a field in
> the pathspec item which indicates the location of the first wildcard character.
> So the prefix (everything prior to the wildcard char) must match
> literally before
> we drop into a more expensive wildmatch function.
"Another part of the code" is about tree walking, right? Weren't
you saying that part of the code may be buggy or something earlier
(e.g. pathspec "su?/" vs entry "sub")?
Again, what do we have in "name" and "item" at this point? If we
have a submodule at "sub/" and we are checking a pathspec element
"sub/dir1/*", what is the non-wildcard part of the pathspec and what
is the "string"? Aren't then "sub/dir1/" and "sub/" respectively,
which would not pass ps_strncmp() and produce a (false) negative?
I am starting to have a feeling that the best we can do in this
function safely is to see if prefix (i.e. the constant part of the
pathspec before the first wildcard) is long enough to cover the
"name" and if "name" part does not match to the directory boundary,
e.g. for this combination
pathspec = "a/b/sib/c/*"
name = "a/b/sub/"
we can say with confidence that it is not worth descending into.
When prefix is long enough and "name" and leading part of the prefix
matches to the directory boundary, e.g.
pathspec = "a/b/sub/c/*"
name = "a/b/sub/"
we can say it is worth descending into.
If these two checks cannot decide, we may have to be pessimistic and
say "it may match; we don't know until we descend into it". When
prefix is shorter than name, I am not sure if we can devise a set of
simple rules, e.g.
pathspec = "a/**/c/*"
name = "a/b/sub/"
may match with its ** "b/sub" part and worth descending into, so is
pathspec = "a/b/*/c/*"
name = "a/b/sub/"
but not this one:
pathspec = "a/b/su[c-z]/c/*"
name = "a/b/sub/"
but this is OK:
pathspec = "a/b/su[a-z]/c/*"
name = "a/b/sub/"
So I would think we'd be in the business of counting slashes in the
name (called "string" in this function) and the pathspec, while
noticing '*' and '**' in the latter, and we may be able to be more
precise, but I am not sure how complex the end result would become.
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-19 16:30 UTC (permalink / raw)
To: Ben Peart; +Cc: pclouds, 'Ben Peart', git
In-Reply-To: <007401d21278$445eba80$cd1c2f80$@gmail.com>
"Ben Peart" <peartben@gmail.com> writes:
> Let me see if I can better explain what I’m trying to accomplish with this
> patch.
>
> "git checkout -b foo" (without -f -m or <start_point>) is defined in the
> manual as being a shortcut for/equivalent to:
>
> (1a) "git branch foo"
> (1b) "git checkout foo"
>
> However, it has been our experience in our observed use cases and all the
> existing git tests, that it can be treated as equivalent to:
>
> (2a) "git branch foo"
> (2b) "git symbolic-ref HEAD refs/heads/foo"
>
> That is, the common perception (use case) is to just create a new branch
> "foo" (pointing at the current commit) and point HEAD at it WITHOUT making
> any changes to the index or worktree.
>
> However, the (1b) command has "git reset" connotations in that it should
> examine and manipulate the trees, index, and worktree in the expectation
> that there MIGHT be work to do.
>
> Since this additional work in (1b) takes minutes on large repos and (2b)
> takes less than a second, my intent was to identify the conditions that this
> additional work will have no affect and thereby avoid it.
>
> Alternatively, was the "-b" option just created as a shortcut only to avoid
> calling the separate "git branch foo" command and we should not think about
> the common perception and usage?
If you are trying to change the definition of "checkout -b" from 1
to 2 above, that is a completely different issue. I thought this
was an attempt to optimize for the performance without changing the
behaviour.
So if you did not apologize like this...
> It is correct that this optimization will skip updating the tree to honor
> any changes to the sparse-checkout in the case of creating a new branch.
> Unfortunately, I don't know of any way to detect the changes other than
> actually doing all the work to update the skip work tree bit in the index.
... but insisted that skipping the yucky sparse-checkout adjustment
in this case was an intended behaviour change, I would have
understood (not necessarily agreed, though) what you were trying to
do.
> Beyond this code review process and testing, I don't know how else we make
> sure we're caught all the conditions where we are OK skipping some of the
> steps. Any change has inherent risk - a change in behavior even more so.
At least we made one-step progress today. I now know that you are
trying to change the behaviour, but I didn't know that last week,
when I was primarily reacting that your claim that this was
performance thing and assuming you meant no change in behaviour, but
there was clearly behaviour change, and it was apparent that the
denseness of the code made it almost impossible to see if there are
unintended changes.
I am still not sure if I like the change of what "checkout -b" is
this late in the game, though.
^ permalink raw reply
* Re: Why are there multiple ways to get the manual in Git?
From: Philip Oakley @ 2016-09-19 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Fredrik Gustafsson, Andrew Johnson, Jakub Narebski, git,
Christian Couder
In-Reply-To: <xmqqlgynhocy.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> The `git revisions --help` does work ;-)
>
> Not anymore ;-)
>
> I think Ralf Thielow fixed it recently.
hmm, I sort of though it would still work with a valid guide.
I'd only checked with my last GfW version.
--
hey ho
Philip
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Junio C Hamano @ 2016-09-19 16:19 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List, Michael J Gruber
In-Reply-To: <033051503D8C4F618B1E4879AFF8C28D@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> At the moment the cherry-pick man page's example implies that
> --do-walk is applied from the beginning, rather from the point given
> on the command line.
>
> I had a very quick search of the *.c code for the options but didn't
> get any further. Hopefully the user issue/misunderstanding is
> elsewhere... I'll add this to my little list.
I think the confusion is coming from not understanding that revision
specifiers cannot have position-dependent semantics, because there
is no "union of multiple sets". You said
commits in 'master..next' range and the tip of 'maint'
earlier, and that is a prime specimen of that confusion. That is
asking "things reachable from next excluding things reachable from
master" computed independently from everything else on the command
line (i.e. that is one set), and "the commit at the tip of 'maint'"
(i.e. that is another set, which consists of a singleton element),
and wanting to take a union of it. But the revision machinery is
not structured to work that way. It can only do "reachable from one
enumeration of positive tips, excluding ones reachable from another
enumeration of negative tips". "no-walk" is a cheap hack that tells
the machinery "stop after collecting that 'one enumeration of
positive tips' and do not walk. Make that enumeration the resulting
set". Having anything negative in the enumeration of starting
points from the command line automatically turns "no-walk" off, even
for commands that default to "no-walk".
We may need further documentation updates to unconfuse readers.
^ permalink raw reply
* Re: Switching branches not working in a cloned repo
From: Philip Oakley @ 2016-09-19 16:16 UTC (permalink / raw)
To: Paul Williamson, git
In-Reply-To: <YQXPR01MB0023CDCFA27BDD4959B4F9B19EF40@YQXPR01MB0023.CANPRD01.PROD.OUTLOOK.COM>
From: "Paul Williamson" <paul.williamson@mediamiser.com>
> Hi,
>
> We use git extensively on a number of repos. Recently, we have had a
> problem with one of them. This repo has a 'web_dev' branch. For copies of
> the repo cloned before a certain (recent but unidentified) time, we could
> 'git checkout' between master and web_dev and everything would be normal.
>
> However, now if we clone the repo, we can no longer do 'git checkout
> web_dev'. Git doesn't complain, in fact there is no feedback and we are
> still in the master branch. Running 'git branch -r' still shows the branch
> as existing at origin.
Have you tried `git ls-remote` ?
The `branch -r` just lists the local 'rtb's (IIUC).
It could be someone has accidently pruned or deleted that branch at the
remote.
What version are you (they) on?
>
> If we try 'git branch web_dev' we then see web_dev listed locally and can
> switch to it BUT on closer inspection we realise that this action has
> created a new branch off master.
>
> The first time we saw this was under Bash on Windows, so we thought maybe
> it was a beta problem, but a) other repos work as expected under that
> environment, and b) under cygwin, pulling the same repo to a new directory
> alongside an older copy shows that the problem occurs with the new clone,
> but not the one that that was cloned longer ago.
>
> Also in this situation, there are no local outstanding code changes that
> might cause problems switching branches. This occurs right from a cleanly
> cloned repo.
>
> It seems something has gone wrong with this repo, and we don't know what.
> It's a tough problem to google, and I was not able to search the gmane
> archives (DNS errors).
Gmane had to quit. Try http://public-inbox.org/git (see the help link)
>
> Any idea how to investigate?
>
> Thanks,
> Paul
--
philip
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Junio C Hamano @ 2016-09-19 16:12 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Philip Oakley, Git List
In-Reply-To: <3b06b9ee-3975-acf1-41d8-02b774a2dd3c@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
>> It can be read that
>>
>> $ git cherry-pick maint next
>>
>> would pick two single commits, while
>>
>> $ git cherry-pick maint next ^master
>>
>> could implicitly be read as
>>
>> $ git cherry-pick maint next --do-walk ^master
You can read it as "master..next maint" that does force walking.
>> Clearly that's not what is intended, which is
>>
>> $ git cherry-pick --do-walk maint next ^master
I do not see the distinction betwee the above two you seem to be
trying to make. Care to explain?
>> but it is open to interpretation as to where in the command line the caret
>> range prefix's --do-walk (to countermand the --no-walk) should applied.
I do not think it can be position dependent. Philip probably has a
confused notion that "rev-list A..B C..D" is somehow a union of set
A..B and C..D?
>> If the user did want just the single commit at the tip of maint, and then
>> the range master..next, what would be their command line, and also, how
>> would the man page warn against false expectations?
Yeah, this can show us that all of the have is coming from that
exact confusion I suspected Philip has. We need to clarify in the
documentation that rev-list set operation does *NOT* have union of
multiple sets to unconfuse the readers.
^ permalink raw reply
* Re: [PATCH v3 0/8] Better heuristics make prettier diffs
From: Junio C Hamano @ 2016-09-19 16:05 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Ramsay Jones, René Scharfe, Stefan Beller, Jeff King,
Jakub Narębski, Jacob Keller
In-Reply-To: <c381d458-4c81-f46c-592a-98957b3a177c@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 09/08/2016 01:25 AM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> * Add test t4059 as part of this commit, not as part of its
>>> successor.
>>
>> Which needs to be moved to somewhere else, as another topics that
>> has already been in 'next' uses t4059.
>>
>> I'd move it temporarily to t4061 with a separate SQUASH??? at the
>> tip for now, as I am running out of time today.
>
> I didn't realize you were waiting for an ACK. Yes, it's totally OK to
> rename the test.
I actually wasn't asking for an Ack.
As the issue was in the one that is buried a few commits from the
tip, and there is a later one that adds more tests to it, I didn't
find enough energy to rename the new file in a buried commit and
then adjust the patch later updates it, I was hoping that you'd
reroll to save me effort, rather than forcing me to do the rebase
myself ;-).
^ permalink raw reply
* Re: Bug: pager.<cmd> doesn't work well with editors
From: Junio C Hamano @ 2016-09-19 16:03 UTC (permalink / raw)
To: Anatoly Borodin, Jeff King; +Cc: git
In-Reply-To: <nrmd6u$imf$1@blaine.gmane.org>
Anatoly Borodin <anatoly.borodin@gmail.com> writes:
>> I think, the pagination should be turned off when the editor is being
>> called.
This is a fun one. IIRC, we decide to spawn a pager and run our
output via pipe thru it fairly early, even before we figure out
which subcommand is being run (especially if you do "git -p
subcommand"), which by definition is way before we know if that
subcommand wants to let the user edit things with the editor.
^ permalink raw reply
* Re: Why are there multiple ways to get the manual in Git?
From: Junio C Hamano @ 2016-09-19 15:59 UTC (permalink / raw)
To: Philip Oakley
Cc: Fredrik Gustafsson, Andrew Johnson, Jakub Narębski, git,
Christian Couder
In-Reply-To: <630656BFF8F84E43A2E4C67138A9A675@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> The `git revisions --help` does work ;-)
Not anymore ;-)
I think Ralf Thielow fixed it recently.
^ permalink raw reply
* Switching branches not working in a cloned repo
From: Paul Williamson @ 2016-09-19 13:58 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
We use git extensively on a number of repos. Recently, we have had a problem with one of them. This repo has a 'web_dev' branch. For copies of the repo cloned before a certain (recent but unidentified) time, we could 'git checkout' between master and web_dev and everything would be normal.
However, now if we clone the repo, we can no longer do 'git checkout web_dev'. Git doesn't complain, in fact there is no feedback and we are still in the master branch. Running 'git branch -r' still shows the branch as existing at origin.
If we try 'git branch web_dev' we then see web_dev listed locally and can switch to it BUT on closer inspection we realise that this action has created a new branch off master.
The first time we saw this was under Bash on Windows, so we thought maybe it was a beta problem, but a) other repos work as expected under that environment, and b) under cygwin, pulling the same repo to a new directory alongside an older copy shows that the problem occurs with the new clone, but not the one that that was cloned longer ago.
Also in this situation, there are no local outstanding code changes that might cause problems switching branches. This occurs right from a cleanly cloned repo.
It seems something has gone wrong with this repo, and we don't know what. It's a tough problem to google, and I was not able to search the gmane archives (DNS errors).
Any idea how to investigate?
Thanks,
Paul
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Philip Oakley @ 2016-09-19 14:46 UTC (permalink / raw)
To: Git List, Michael J Gruber
In-Reply-To: <3b06b9ee-3975-acf1-41d8-02b774a2dd3c@drmicha.warpmail.net>
From: "Michael J Gruber" <git@drmicha.warpmail.net>
> Philip Oakley venit, vidit, dixit 19.09.2016 12:56:
>> A question came up on the Git user list regarding cherry-pick that got me
>> reading the manual (again), in this case regarding --no-walk ranges.
>>
>> Essentially my question is: If --no-walk is given to rev-list (e.g. via
>> charry-pick), and the user includes a caret prefixed rev, when does that
>> range definition take effect on the command line, especially in light of
>> the --do-walk option?
>>
>> In rev-list(1) there are only 8 references to 'range', with only
>> the --no-walk option saying "This has no effect if a range is specified."
>> but leaving open the decision as to what does (and does not) comprises
>> the
>> specification of a range on the cli.
>>
>> The two and three dot notations are fairly obvious ranges from
>> gitrevisions(7) as they are complete strings, while the caret prefix is
>> an
>> implied range (it needs additional parameters to complete the range, and
>> there-in lies the issue).
>>
>> It can be read that
>>
>> $ git cherry-pick maint next
>>
>> would pick two single commits, while
>>
>> $ git cherry-pick maint next ^master
>>
>> could implicitly be read as
>>
>> $ git cherry-pick maint next --do-walk ^master
>>
>> because the ^ caret starts the range that cancels the --no-walk.
>>
>> Clearly that's not what is intended, which is
>>
>> $ git cherry-pick --do-walk maint next ^master
>>
>> but it is open to interpretation as to where in the command line the
>> caret
>> range prefix's --do-walk (to countermand the --no-walk) should applied.
>>
>> If the user did want just the single commit at the tip of maint, and then
>> the range master..next, what would be their command line, and also, how
>> would the man page warn against false expectations?
>
> Maybe:
>
> Every negative rev (rev prefixed with ^, or a range) implies a
> `--do-walk` (right at its position on the command line).
>
> And then curb the misleading range sentence in the `--no-walk`
> description.
At the moment the cherry-pick man page's example implies that --do-walk is
applied from the beginning, rather from the point given on the command line.
I had a very quick search of the *.c code for the options but didn't get any
further. Hopefully the user issue/misunderstanding is elsewhere... I'll add
this to my little list.
--
Philip
^ permalink raw reply
* [Bug] Custom git-dir directory shouldn't be listed as “untracked”
From: Nicolas Cuillery @ 2016-09-19 14:26 UTC (permalink / raw)
To: git
Hi, I want to create a local repository with a custom dir name instead
of ".git", I used the env vars GIT_DIR and WORK_TREE:
>export GIT_DIR=".customgitdir"
>export GIT_WORK_TREE="."
Then I created a repo in an empty directory:
>$ git init
>Initialized empty Git repository in XXXXXXXXX/.customgitdir/
Then I ran git status:
>$ git status
>On branch master
>
>Initial commit
>
>Untracked files:
> (use "git add <file>..." to include in what will be committed)
>
> .customgitdir/
>
>nothing added to commit but untracked files present (use "git add" to track)
The local repo directory listed as "untracked files" which is a
problem when using "git add ." afterwards.
When using the default directory ".git", it logically doesn't appear
in the "git status" command's output. Don't you think it should be the
same when using a custom dir name ?
Git version 2.6.4 on MacOSX 10.11
Regards,
Nicolas
^ permalink raw reply
* Re: [PATCH 11/11] Resumable clone: implement primer logic in git-clone
From: Duy Nguyen @ 2016-09-19 14:04 UTC (permalink / raw)
To: Kevin Wern; +Cc: Git Mailing List
In-Reply-To: <1473984742-12516-12-git-send-email-kevin.m.wern@gmail.com>
On Fri, Sep 16, 2016 at 7:12 AM, Kevin Wern <kevin.m.wern@gmail.com> wrote:
> builtin/clone.c | 590 +++++++++++++++++++++++++++++++++++++-------
Argh.. this is too big for my brain at this hour. It might be easier
to follow if you separate out some code move (I think I've seen some,
not sure). I'll try to have another look when I find time. But it's
great to hear from you again, the pleasant surprise in my inbox today,
as I thought we lost you ;-) There's hope for resumable clone maybe
before 2018 again.
--
Duy
^ permalink raw reply
* Re: [PATCH 04/11] Resumable clone: add prime-clone to remote-curl
From: Duy Nguyen @ 2016-09-19 13:52 UTC (permalink / raw)
To: Kevin Wern; +Cc: Git Mailing List
In-Reply-To: <1473984742-12516-5-git-send-email-kevin.m.wern@gmail.com>
On Fri, Sep 16, 2016 at 7:12 AM, Kevin Wern <kevin.m.wern@gmail.com> wrote:
> +static void prime_clone(void)
> +{
> + char *result, *result_full, *line;
> + size_t result_len;
> + int err = 0, one_successful = 0;
> +
> + if (request_service("git-prime-clone", &result_full, &result,
> + &result_len, HTTP_ERROR_GENTLE)) {
> + while (line = packet_read_line_buf_gentle(&result, &result_len,
> + NULL)) {
> + char *space = strchr(line ,' ');
> +
> + // We will eventually support multiple resources, so
> + // always parse the whole message
> + if (err)
> + continue;
> + if (!space || strchr(space + 1, ' ')) {
> + if (options.verbosity > 1)
> + fprintf(stderr, "prime clone "
> + "protocol error: got '%s'\n",
> + line);
> + printf("error\n");
> + err = 1;
> + continue;
> + }
> +
> + one_successful = 1;
> + printf("%s\n", line);
A brief overview for this service in
Documentation/technical/http-protocol.txt (and maybe
Documentation/gitremote-helpers.txt as well) would be great help. It's
a bit hard to follow because at this point I don't know anything about
the server side (and on top of that I was confused between http
send/receive vs transport send/receive, but this is my fault).
> + }
> + if (!one_successful && options.verbosity > 1)
> + fprintf(stderr, "did not get required components for "
> + "alternate resource\n");
> + }
> +
> + printf("\n");
> + fflush(stdout);
> + free(result_full);
> +}
--
Duy
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Michael J Gruber @ 2016-09-19 13:46 UTC (permalink / raw)
To: Philip Oakley, Git List
In-Reply-To: <2AD952BD65034D25BF26C7F138D24F25@PhilipOakley>
Philip Oakley venit, vidit, dixit 19.09.2016 12:56:
> A question came up on the Git user list regarding cherry-pick that got me
> reading the manual (again), in this case regarding --no-walk ranges.
>
> Essentially my question is: If --no-walk is given to rev-list (e.g. via
> charry-pick), and the user includes a caret prefixed rev, when does that
> range definition take effect on the command line, especially in light of
> the --do-walk option?
>
> In rev-list(1) there are only 8 references to 'range', with only
> the --no-walk option saying "This has no effect if a range is specified."
> but leaving open the decision as to what does (and does not) comprises the
> specification of a range on the cli.
>
> The two and three dot notations are fairly obvious ranges from
> gitrevisions(7) as they are complete strings, while the caret prefix is an
> implied range (it needs additional parameters to complete the range, and
> there-in lies the issue).
>
> It can be read that
>
> $ git cherry-pick maint next
>
> would pick two single commits, while
>
> $ git cherry-pick maint next ^master
>
> could implicitly be read as
>
> $ git cherry-pick maint next --do-walk ^master
>
> because the ^ caret starts the range that cancels the --no-walk.
>
> Clearly that's not what is intended, which is
>
> $ git cherry-pick --do-walk maint next ^master
>
> but it is open to interpretation as to where in the command line the caret
> range prefix's --do-walk (to countermand the --no-walk) should applied.
>
> If the user did want just the single commit at the tip of maint, and then
> the range master..next, what would be their command line, and also, how
> would the man page warn against false expectations?
Maybe:
Every negative rev (rev prefixed with ^, or a range) implies a
`--do-walk` (right at its position on the command line).
And then curb the misleading range sentence in the `--no-walk` description.
Michael
^ permalink raw reply
* Re: [PATCH 09/11] path: add resumable marker
From: Duy Nguyen @ 2016-09-19 13:24 UTC (permalink / raw)
To: Kevin Wern; +Cc: Git Mailing List
In-Reply-To: <1473984742-12516-10-git-send-email-kevin.m.wern@gmail.com>
On Fri, Sep 16, 2016 at 7:12 AM, Kevin Wern <kevin.m.wern@gmail.com> wrote:
> Create function to get gitdir file RESUMABLE.
A very good opportunity to explain what this file is or will be (and
whether its content matters or just its existence). Either as commit
message, or even better in Documentation/gitrepository-layout.txt.
--
Duy
^ permalink raw reply
* RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-19 13:18 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: pclouds, peartben, 'Ben Peart', git
In-Reply-To: <BL2PR03MB323E1B2F810C63CB01AA234F4F30@BL2PR03MB323.namprd03.prod.outlook.com>
Let me see if I can better explain what I’m trying to accomplish with this
patch.
"git checkout -b foo" (without -f -m or <start_point>) is defined in the
manual as being a shortcut for/equivalent to:
(1a) "git branch foo"
(1b) "git checkout foo"
However, it has been our experience in our observed use cases and all the
existing git tests, that it can be treated as equivalent to:
(2a) "git branch foo"
(2b) "git symbolic-ref HEAD refs/heads/foo"
That is, the common perception (use case) is to just create a new branch
"foo" (pointing at the current commit) and point HEAD at it WITHOUT making
any changes to the index or worktree.
However, the (1b) command has "git reset" connotations in that it should
examine and manipulate the trees, index, and worktree in the expectation
that there MIGHT be work to do.
Since this additional work in (1b) takes minutes on large repos and (2b)
takes less than a second, my intent was to identify the conditions that this
additional work will have no affect and thereby avoid it.
Alternatively, was the "-b" option just created as a shortcut only to avoid
calling the separate "git branch foo" command and we should not think about
the common perception and usage?
More comments inline...
> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, September 13, 2016 6:35 PM
> To: Ben Peart <mailto:peartben@gmail.com>
> Cc: mailto:git@vger.kernel.org; mailto:pclouds@gmail.com; Ben Peart
> <mailto:Ben.Peart@microsoft.com>
> Subject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial
> checkout
>
> Ben Peart <mailto:peartben@gmail.com> writes:
>
> > +static int needs_working_tree_merge(const struct checkout_opts *opts,
> > + const struct branch_info *old,
> > + const struct branch_info *new)
> > +{
> > +...
> > +}
>
> I do not think I need to repeat the same remarks on the conditions in this
> helper, which hasn't changed since v2. Many "comments" in the code do not
> explain why skipping is justified, or what they claim to check looks to me
just
> plain wrong.
>
> For example, there is
>
> /*
> * If we're not creating a new branch, by definition we're changing
> * the existing one so need to do the merge
> */
> if (!opts->new_branch)
> return 1;
>
> but "git checkout" (no other argument) hits this condition. It disables
the
> most trivial optimization opportunity, because we are not "creating".
>
Disabling the optimization for "git checkout" with no argument was
intentional. This command does not create a new branch but instead, performs
a "soft reset" which will update the index and working directory to reflect
changes to the sparse-checkout (for example). If this was not disabled,
many tests fail as they expect this behavior. Because "git checkout" does
not actually change the refs, if we skipped the merge/index/working
directory update, this command becomes a no-op.
> "By definition, we're changing"? Really? Not quite.
>
What I was attempting to communicate is that if we aren't creating a new
branch any changes or updates will happen in the existing branch. Since
that could only be updating the index and working directory, we don't want
to skip those steps or we've defeated any purpose in running the command.
> If you disable this bogus check, "git checkout" (no other argument) would
be
> allowed to skip the merge_working_tree(), and that in turn reveals another
> case that the helper is not checking when
> unpack_trees() MUST be called.
>
> Note: namely, when sparse checkout is in effect, switching from
> HEAD to HEAD can nuke existing working tree files outside the
> sparse pattern -- YUCK! See penultimate test in t1011 for
> an example.
>
> This yuckiness is not your fault, but needs_working_tree_merge() logic you
> added needs to refrain from skipping unpack_trees() call when sparse thing
> is in effect. I'd expect "git checkout -b foo"
> instead of "git checkout" (no other argument) would fail to honor the
sparse
> thing and reveal this bug, because the above bogus "!opts->new_branch"
> check will not protect you for that case.
>
It is correct that this optimization will skip updating the tree to honor
any changes to the sparse-checkout in the case of creating a new branch.
Unfortunately, I don't know of any way to detect the changes other than
actually doing all the work to update the skip work tree bit in the index.
If this behavior is required, then this optimization will need to check if
sparse-checkout is enabled and skip the optimization just in case there have
been changes.
> In other words, these random series of "if (...) return 1" are bugs hiding
> other real bugs and we need to reason about which ones are bugs that are
> hiding what other bugs that are not covered by this function. As Peff
said
> earlier for v1, this is still an unreadable mess. We need to figure out a
way to
> make sure we are skipping on the right condition and not accidentally
hiding
> a bug of failing to check the right condition. I offhand do not have a
good
> suggestion on this; sorry.
>
Beyond this code review process and testing, I don't know how else we make
sure we're caught all the conditions where we are OK skipping some of the
steps. Any change has inherent risk - a change in behavior even more so.
> > static int merge_working_tree(const struct checkout_opts *opts,
> > struct branch_info *old,
> > struct branch_info *new,
> > int *writeout_error)
> > {
> > + /*
> > + * Optimize the performance of "git checkout -b foo" by avoiding
> > + * the expensive merge, index and working directory updates if
they
> > + * are not needed.
> > + */
> > + if (!needs_working_tree_merge(opts, old, new))
> > + return 0;
> > +
> > int ret;
> > struct lock_file *lock_file = xcalloc(1, sizeof(struct
lock_file));
>
> With the change you made at the beginning of this function, it no longer
> compiles with -Wdecl-after-stmt, but that is the smallest of the problems.
I apologize, I didn't realize this was a requirement. It built and passed
all existing tests on Windows but I will reorder the declarations to prevent
causing issues with other platforms/compilers.
>
> It is a small step in the right direction to move the call to the helper
from the
> caller to this function, but it is a bit too small.
>
> Notice that the lines after the above context look like this:
>
> hold_locked_index(lock_file, 1);
> if (read_cache_preload(NULL) < 0)
> return error(_("index file corrupt"));
>
> resolve_undo_clear();
> if (opts->force) {
> ret = reset_tree(new->commit->tree, opts, 1,
> writeout_error);
> if (ret)
> return ret;
> } else {
> struct tree_desc trees[2];
> ...
>
> I would have expected that the check goes inside the "else" thing that
> actually does a two-tree merge, and the helper loses the check with opts-
> >force, at least. That would still be a change smaller than desired, but
at
> least a meaningful improvement compared to the previous one.
I'll restructure it that way.
> As I have
> already pointed out, in the "else" clause there is a check "is the index
free of
> conflicted entries? if so error out", and that must be honored in
!opt->force
> case, no matter what your needs_working_tree_merge() says.
Given we're not merging trees, updating the index, or work tree, why do we
need to error out in this case? We aren't attempting this optimization if
they pass "-m." If there are conflicted entries that haven't been fixed,
they will still exist. We're essentially just creating a new reference for
the existing commit/index/work tree.
> I also was
> hoping that you would notice, when you were told about the unmerged
> check, by reading the remainder of the merge_working_tree(), that we need
> to call show_local_changes() when we are not doing force and when we are
> not quiet---returning early like the above patch will never be able to
call that
> one downstream in the function.
It is a good point that my optimization skipped the call to
show_local_changes. Thanks for catching that, I've fixed it for my next
iteration.
>
> Regardless of what the actual checks end up to be, the right place to do
this
> "optimization" would look more like:
>
> builtin/checkout.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c index
2b50a49..a6b9e17
> 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -508,14 +508,19 @@ static int merge_working_tree(const struct
> checkout_opts *opts,
> topts.dir->flags |=
DIR_SHOW_IGNORED;
>
setup_standard_excludes(topts.dir);
> }
> +
> + if ( we know we can skip the unpack ) {
> + ret = 0;
> + } else {
> tree =
parse_tree_indirect(old->commit ?
>
old->commit-
> >object.oid.hash :
>
EMPTY_TREE_SHA1_BIN);
> init_tree_desc(&trees[0],
tree->buffer, tree->size);
> tree =
parse_tree_indirect(new->commit-
> >object.oid.hash);
> init_tree_desc(&trees[1],
tree->buffer, tree->size);
> -
> ret = unpack_trees(2, trees,
&topts);
> + }
> +
> if (ret == -1) {
> /*
> * Unpack couldn't do a
trivial merge; either
>
I'll restructure it to be like you suggest above however, given we will not
be merging the tress, we won't have any index changes to write out. I will
also skip the calls to cache_tree_update and write_locked_index.
> I'd think. Note that the determination of "we can skip" would involve
> knowing the object names of the two trees involved, so for performance
> reasons, some of the parse-tree calls may have to come before the call to
> "do we know we can skip?", but that does not fundamentally change the
> basic code structure.
>
> Thanks.
I don't understand why we'd need to know the object names of the two trees
given we have the IDs. What did you have in mind that would need those?
^ permalink raw reply
* Re: git add --intent-to-add silently creates empty commits
From: Duy Nguyen @ 2016-09-19 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Aviv Eyal, Git Mailing List
In-Reply-To: <xmqq1t0loxz4.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 16, 2016 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Aviv Eyal <avivey@gmail.com> writes:
>
>> Using `git add -N` allows creating of empty commits:
>>
>> git init test && cd test
>> echo text > file
>> git add --intent-to-add file
>> git commit -m 'Empty commit'
>> echo $? # prints 0
>> ...
>> I'd expect `git commit` to error out instead of producing an empty commit.
>>
>> I've seen this with git 2.8.1 and 2.10.0.129.g35f6318
>
> I think I've seen this reported some time ago.
>
> https://public-inbox.org/git/%3CCACsJy8A8-RgpYxYsJBaLrMia7D3DfQPr4cxASNsaLyCnmgm3ZQ@mail.gmail.com%3E/
>
> I do not offhand recall what happend to the topic after that.
Yeah. I'm a bit behind, no, I'm waaaay behind my git backlog. This
definitely gets a rise-up, together with the multiworktree bug fix in
git-init.
--
Duy
^ permalink raw reply
* Re: [PATCH 02/11] Resumable clone: add prime-clone endpoints
From: Duy Nguyen @ 2016-09-19 13:15 UTC (permalink / raw)
To: Kevin Wern; +Cc: Git Mailing List
In-Reply-To: <1473984742-12516-3-git-send-email-kevin.m.wern@gmail.com>
On Fri, Sep 16, 2016 at 7:12 AM, Kevin Wern <kevin.m.wern@gmail.com> wrote:
> static struct daemon_service daemon_service[] = {
> { "upload-archive", "uploadarch", upload_archive, 0, 1 },
> { "upload-pack", "uploadpack", upload_pack, 1, 1 },
> { "receive-pack", "receivepack", receive_pack, 0, 1 },
> + { "prime-clone", "primeclone", prime_clone, 0, 1 },
> };
I guess this is why you chose to implement a new command in 01/11,
simpler to be called from http-backend?
> + // prime-clone does not need --stateless-rpc and
> + // --advertise-refs options. Maybe it will in the future, but
> + // until then it seems best to do this instead of adding
> + // "dummy" options.
Stick to /* .. */
> + if (strcmp(svc->name, "prime-clone") != 0) {
> + argv_array_pushl(&argv, "--stateless-rpc",
> + "--advertise-refs", NULL);
> + }
We also have an exception for select_getanyfile() below. I think it's
time we add a function callback in struct rpc_service to run each
service the way they want. Then prime-clone won't need an exception
(neither does select_anyfile, mostly)
--
Duy
^ permalink raw reply
* [PATCH 6/6] i18n: stash: mark messages for translation
From: Vasco Almeida @ 2016-09-19 13:08 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1474290501-2743-1-git-send-email-vascomalmeida@sapo.pt>
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
git-stash.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 826af18..90d63f2 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -100,7 +100,7 @@ create_stash () {
u_tree=$(git write-tree) &&
printf 'untracked files on %s\n' "$msg" | git commit-tree $u_tree &&
rm -f "$TMPindex"
- ) ) || die "Cannot save the untracked files"
+ ) ) || die "$(gettext "Cannot save the untracked files")"
untracked_commit_option="-p $u_commit";
else
@@ -248,7 +248,7 @@ save_stash () {
if test -n "$patch_mode" && test -n "$untracked"
then
- die "Can't use --patch and --include-untracked or --all at the same time"
+ die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
stash_msg="$*"
@@ -494,7 +494,7 @@ apply_stash () {
GIT_INDEX_FILE="$TMPindex" git-read-tree "$u_tree" &&
GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
rm -f "$TMPindex" ||
- die 'Could not restore untracked files from stash'
+ die "$(gettext "Could not restore untracked files from stash")"
fi
eval "
--
2.7.4
^ permalink raw reply related
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