Re: [PATCH 4/4] config: don't implicitly use gitdir
On Mon, Jun 12, 2017 at 11:16:27PM -0700, Brandon Williams wrote:
> > > *puzzled* Why wasn't this needed before, then? The rest of the patch
> > > should result in no functional change, but this part seems different.
> >
> > Now I'm puzzled, too. The original that got filled in lazily by the
> > config functions was always get_git_dir(). I can buy the argument that
> > this was a bug (I'm not familiar enough with worktree to say one way or
> > the other), but if it's a fix it should definitely go into another
> > patch.
>
> Well actually... in do_git_config_sequence 'git_path("config")' is
> called which will convert gitdir to commondir under the hood. you can't
> use vanilla gitdir because the config isn't stored in a worktree's
> gitdir but rather in the commondir as the config is shared by all
> worktrees.
Sorry, I missed the fact that there were two sites changed on the first
read.
> So maybe we actually need to add a field to the 'config_options' struct
> of 'commondir' such that the commondir can be used to load the actual
> config file and 'gitdir' can be used to handle the 'IncludeIf' stuff.
On reflection, I suspect that probably is the case. If you have a
workdir in ~/foo, you probably want to match IncludeIf against that
instead of wherever the common dir happens to be.
-Peff
Re: [PATCH] Fix KeyError "fileSize" in verbose mode
> On 12 Jun 2017, at 19:11, Junio C Hamano wrote:
>
> Sergey Yurzin writes:
>
>> Subject: Re: [PATCH] Fix KeyError "fileSize" in verbose mode
>
> ...
>
>> git-p4.py | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 8d151da91b969..b3666eddf12e3 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents):
>> relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
>> relPath = self.encodeWithUTF8(relPath)
>> if verbose:
>> -size = int(self.stream_file['fileSize'])
>> -sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'],
>> relPath, size/1024/1024))
>> +if 'fileSize' in self.stream_file:
>> +size = int(self.stream_file['fileSize'])
>> +sys.stdout.write('\r%s --> %s (%i MB)\n' %
>> (file['depotFile'], relPath, size/1024/1024))
>> +else:
>> +sys.stdout.write('\r%s --> %s\n' % (file['depotFile'],
>> relPath))
>> sys.stdout.flush()
>
Are you working with P4 Streams? I think I ran into the same problem
if Streams are involved and I solved it like this:
if verbose:
-size = int(self.stream_file['fileSize'])
+if 'fileSize' in self.stream_file:
+size = int(self.stream_file['fileSize'])
+elif 'streamContentSize' in self.stream_file:
+size = int(self.stream_file['streamContentSize'])
+else:
+size = 0
sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'],
relPath, size/1024/1024))
sys.stdout.flush()
However "streamContentSize" is something we define ourselves elsewhere
in git-p4 and not something we get from P4. Therefore it might be
garbage. I haven't looked into it further, yet.
Your 'fileSize' check is definitively correct!
Cheers,
Lars
> I can see from your patch that self.stream_file[] sometimes may not
> have `fileSize` and when that happens the current code will barf. I
> also can see that with your patch, the code will NOT barf but output
> would lack the size information (obviously, because it is not
> available).
>
> However, it is not at all obvious if this is fixing the problem or
> sweeping the problem under the rug. The proposed log message you
> write before the patch is the ideal place to say something like
>
>In such and such circumstances, it is perfectly normal that
>P4Sync.stream_file does not know its file and lacks `fileSize`.
>streamOneP4File() method, however, assumes that this key is
>always available and tries to write it under the verbose mode.
>
>Check the existence of the `fileSize` key and show it only when
>available.
>
> Note that the above _assumes_ that a stream_file that lacks
> `fileSize` is perfectly normal; if that assumption is incorrect,
> then perhaps a real fix may be to set `fileSize` in the codepath
> that forgets to set it (I am not a git-p4 expert, and I am asking
> Luke to review this patch by CC'ing him).
>
> Also note that in a real log message that is helpful for future
> readers, "In such and such circumstances" in the above illustration
> needs to become a more concrete description.
>
> Thanks, and welcome to Git development community.
>
>> (type_base, type_mods) = split_p4_type(file["type"])
>>
>> --
>> https://github.com/git/git/pull/373
Re: [PATCH v1] Configure Git contribution guidelines for github.com
> On 12 Jun 2017, at 17:52, Junio C Hamano wrote: > > "Philip Oakley" writes: > >> From: "Lars Schneider" >>> Many open source projects use github.com for their contribution process. >>> Although we mirror the Git core repository to github.com [1] we do not >>> use any other github.com service. This is unknown/unexpected to a >>> number of (potential) contributors and consequently they create Pull >>> Requests against our mirror with their contributions. These Pull >>> Requests become stall [2]. This is frustrating to them as they think we >>> ignore them and it is also unsatisfactory for us as we miss potential >>> code improvements and/or new contributors. >>> >>> GitHub offers a way to notify Pull Request contributors about the >>> contribution guidelines for a project [3]. Let's make use of this! >>> >>> [1] https://github.com/git/git >>> [2] https://github.com/git/git/pulls >>> [3] >>> https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ >>> >>> Signed-off-by: Lars Schneider >>> --- >> >> I see there are currently 84 open PRs (13 in the last 14 days), so it >> is real. > > Not so insignificant fraction of these are done purely for the > purpose of using submitgit, though. In other words, if submitgit > were improved not to require a pull request against [1] (instead, it > could be pointed at a branch in a contributor's repository and do > the fromatting), these numbers will go down. As an alternative, could SubmitGit close the Pull Request automatically after the mails have been send successfully? >>> +Thanks for taking the time to contribute to Git! Please be advised, >>> that the Git community does not use github.com for their >>> contributions. Instead, we use a [mailing >>> list](http://public-inbox.org/git/) for code submissions, code >>> reviews, and bug reports. >> >> Isn't the mailing list git@vger.kernel.org, with an archive at >> http://public-inbox.org/git/ ? >> > > I agree that I found the URL of the archive somewhat misleading for > those who want to contribute. Giving the mailing list address makes > a lot more sense. As suggested by Ævar in the thread, it would also > be good to suggest submitgit, given that the target audience of this > message was already prepared to throw a pull request at us. OK, will fix! >>> +Please [read the maintainer >>> notes](http://repo.or.cz/w/git.git?a=blob_plain;f=MaintNotes;hb=todo) >>> to learn how the Git > > Two minor issues here. > > 1. push the "read" outside [], i.e. > > Please read []( the thing>) to learn... > > as what is inside [] and what is inside () ought to be the moral > equivalents. > > 2. the thing is not called "the maintainer notes"; it is called "A > note from the maintainer". OK, will fix! >> Is using the repo.or.cz address deliberate as a way of highlighting >> that Github isn't the centre of the universe when accessing a DVCS >> repo? >> >> Maybe the kernel.org repo should be first, or at least the alt-git.git >> repo at repo.or.cz listed in those same notes. > > I'd prefer [the k.org > address](https://git.kernel.org/pub/scm/git/git.git/plain/MaintNotes?h=todo). OK, will fix! > >>> +project is managed, and how you can work with it. In addition, we >>> highly recommend you to [read our submission >>> guidelines](../Documentation/SubmittingPatches). > > Again, push "read our" outside []. OK, will fix! Thanks, Lars
Re: [PATCH v1] Configure Git contribution guidelines for github.com
> On 10 Jun 2017, at 14:48, Philip Oakley wrote: > > From: "Lars Schneider" >> Many open source projects use github.com for their contribution process. >> Although we mirror the Git core repository to github.com [1] we do not >> use any other github.com service. This is unknown/unexpected to a >> number of (potential) contributors and consequently they create Pull >> Requests against our mirror with their contributions. These Pull >> Requests become stall [2]. This is frustrating to them as they think we >> ignore them and it is also unsatisfactory for us as we miss potential >> code improvements and/or new contributors. >> >> GitHub offers a way to notify Pull Request contributors about the >> contribution guidelines for a project [3]. Let's make use of this! >> >> [1] https://github.com/git/git >> [2] https://github.com/git/git/pulls >> [3] >> https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ >> >> Signed-off-by: Lars Schneider >> --- > > I see there are currently 84 open PRs (13 in the last 14 days), so it is real. > > I also see that the Issues page for git.git appears to be disabled, and will > redirect to the pulls page. > > Maybe the instructions should also be part of an Issues template which could > reduce the potential number of PRs being created (but could create its own > problems) I don't think that is necessary as Issues are disabled for this repo. >> ... >> +Thanks for taking the time to contribute to Git! Please be advised, that >> the Git community does not use github.com for their contributions. Instead, >> we use a [mailing list](http://public-inbox.org/git/) for code submissions, >> code reviews, and bug reports. > > Isn't the mailing list git@vger.kernel.org, with an archive at > http://public-inbox.org/git/ ? Agreed! > >> + >> +Please [read the maintainer >> notes](http://repo.or.cz/w/git.git?a=blob_plain;f=MaintNotes;hb=todo) to >> learn how the Git > > Is using the repo.or.cz address deliberate as a way of highlighting that > Github isn't the centre of the universe when accessing a DVCS repo? Haha, partly yes. Plus, I wasn't able to figure out quickly how to access the todo blob on GitHub. > Maybe the kernel.org repo should be first, or at least the alt-git.git repo > at repo.or.cz listed in those same notes. Agreed! > It's still a good idea though. Thanks :) - Lars
Re: [PATCH v1] Configure Git contribution guidelines for github.com
> On 10 Jun 2017, at 09:35, Jeff King wrote: > > On Fri, Jun 09, 2017 at 10:03:57AM -0700, Jonathan Nieder wrote: > >> Would putting a PULL_REQUEST_TEMPLATE and CONTRIBUTING in the >> top-level directory work? If I'm reading >> https://help.github.com/articles/setting-guidelines-for-repository-contributors/#adding-a-contributing-file >> correctly then it seems to say the ".github/" prefix is optional. > > Yes, that should work. The ".github" option is there if you don't want > the cruft at your top-level. I don't mind CONTRIBUTING, but > PULL_REQUEST_TEMPLATE is kind of gross. :) I think so, too :) >> I also find the long source lines hard to read. Can you say more >> about how broken lines render in the github.com web interface? I >> would have expected github's markdown renderer to behave like others >> and cope with line wrapping. > > GitHub-Flavored Markdown treats newlines as a line break in the rendered > output, unlike the original Markdown implementation. I don't like that > myself, but it was done in the early days after seeing how many people > accidentally butchered their formatting because they expected the output > to look more like what they typed. That's what I had in my memory based on past experience, too. However, out of curiosity I just tried it and it looks like they have changed it. I'll reformat with 80 chars. - Lars
Re: [PATCH v1] Configure Git contribution guidelines for github.com
> On 10 Jun 2017, at 03:51, Junio C Hamano wrote: > > Jonathan Nieder writes: > >> Lars Schneider wrote: >> >>> Many open source projects use github.com for their contribution process. >>> Although we mirror the Git core repository to github.com [1] we do not >>> use any other github.com service. This is unknown/unexpected to a >>> number of (potential) contributors and consequently they create Pull >>> Requests against our mirror with their contributions. These Pull >>> Requests become stall [2]. This is frustrating to them as they think we >>> ignore them and it is also unsatisfactory for us as we miss potential >>> code improvements and/or new contributors. >> >> I think this description could be more focused. It's also not >> self-contained --- e.g. the link to stalled pull requests is likely to >> become stale over time, especially if GitHub gives us a way to disable >> pull requests for the repository some day. >> >> Could you summarize for me the motivation behind this patch? Is it to >> make Git more approachable, to avoid frustrating contributors, etc? > > I wonder if s/stall/stale/ is what Lars meant. Of course :) - Lars
[PATCH v2] Configure Git contribution guidelines for github.com
Many open source projects use github.com for their contribution process. Although we mirror the Git core repository to github.com [1] we do not use any other github.com service. This is unknown/unexpected to a number of (potential) contributors and consequently they create Pull Requests against our mirror with their contributions. These Pull Requests become stale. This is frustrating to them as they think we ignore them and it is also unsatisfactory for us as we miss potential code improvements and/or new contributors. GitHub contribution guidelines and a GitHub Pull Request template that is visible to every Pull Request creator can be configured with special files in a Git repository [2]. Let's make use of this! [1] https://github.com/git/git [2] https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ Signed-off-by: Lars Schneider --- Hi, changes since v1: * mention submitGit * link to mailing list address instead of mailing list archive * reformat long lines * move "read" out of link title * use kernel.org repo for "A note from the maintainer" * reference "A note from the maintainer" with the correct name You can see a preview here: https://github.com/larsxschneider/git/compare/master...larsxschneider-patch-1?quick_pull=1 Thanks, Lars Notes: Base Ref: master Web-Diff: https://github.com/larsxschneider/git/commit/c438291da7 Checkout: git fetch https://github.com/larsxschneider/git contrib-guide-v2 && git checkout c438291da7 .github/CONTRIBUTING.md | 19 +++ .github/PULL_REQUEST_TEMPLATE.md | 7 +++ 2 files changed, 26 insertions(+) create mode 100644 .github/CONTRIBUTING.md create mode 100644 .github/PULL_REQUEST_TEMPLATE.md diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md new file mode 100644 index 00..fc397bb8fc --- /dev/null +++ b/.github/CONTRIBUTING.md @@ -0,0 +1,19 @@ +## Contributing to Git + +Thanks for taking the time to contribute to Git! Please be advised, that the +Git community does not use github.com for their contributions. Instead, we use +a mailing list (git@vger.kernel.org) for code submissions, code +reviews, and bug reports. + +Nevertheless, you can use [submitGit](http://submitgit.herokuapp.com/) to +conveniently send your Pull Requests commits to our mailing list. + +Please read ["A note from the maintainer"](https://git.kernel.org/pub/scm/git/git.git/plain/MaintNotes?h=todo) +to learn how the Git project is managed, and how you can work with it. +In addition, we highly recommend you to read [our submission guidelines](../Documentation/SubmittingPatches). + +If you prefer video, then [this talk](https://www.youtube.com/watch?v=Q7i_qQW__q4&feature=youtu.be&t=6m4s) +might be useful to you as the presenter walks you through the contribution +process by example. + +Your friendly Git community! diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 00..2b617f4c25 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,7 @@ +Thanks for taking the time to contribute to Git! Please be advised, that the +Git community does not use github.com for their contributions. Instead, we use +a mailing list (git@vger.kernel.org) for code submissions, code reviews, and +bug reports. Nevertheless, you can use submitGit to conveniently send your Pull +Requests commits to our mailing list. + +Please read the "guidelines for contributing" linked above! base-commit: 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b -- 2.13.0
Re: [PATCH v2] Configure Git contribution guidelines for github.com
On Tue, Jun 13, 2017 at 10:18:07AM +0200, Lars Schneider wrote: > changes since v1: > * mention submitGit > * link to mailing list address instead of mailing list archive You might want to link to https://git-scm.com/community/, which has a section on the mailing list at the top. It gives the list address but also talks about the archive, that you can send to it without subscribing, etc. > * reformat long lines I think this is OK. For CONTRIBUTING.md, GitHub re-wraps. In the pull request template itself, though, the newlines become hard-wraps. But we wouldn't generally expect those lines to remain in the final PR text anyway, so it shouldn't really matter. That's actually one annoyance I have with PR and issue templates like this: the submitter has to manually delete them. But there's not really a better way to get people's attention. The link to CONTRIBUTING is by far not enough in my experience (I had to put a template in git/git-scm.com for people to top submitting Git bugs). The text itself looks good, but two minor grammar nits: > diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md > new file mode 100644 > index 00..fc397bb8fc > --- /dev/null > +++ b/.github/CONTRIBUTING.md > @@ -0,0 +1,19 @@ > +## Contributing to Git > + > +Thanks for taking the time to contribute to Git! Please be advised, that the > +Git community does not use github.com for their contributions. Instead, we > use > +a mailing list (git@vger.kernel.org) for code submissions, code > +reviews, and bug reports. I think the comma after "advised" is unnecessary (you could also drop "that" to turn the first part into an introductory clause, but I think it reads better with as "Please be advised that"). > diff --git a/.github/PULL_REQUEST_TEMPLATE.md > b/.github/PULL_REQUEST_TEMPLATE.md > new file mode 100644 > index 00..2b617f4c25 > --- /dev/null > +++ b/.github/PULL_REQUEST_TEMPLATE.md > @@ -0,0 +1,7 @@ > +Thanks for taking the time to contribute to Git! Please be advised, that the Ditto here. -Peff
Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone
On Fri, Jun 09, 2017 at 07:19:35PM -0400, Jeff King wrote: > Should "git add" check whether there's a matching .gitmodules entry for > the path and issue a warning otherwise? Here's my attempt at that. [1/2]: add: warn when adding an embedded repository [2/2]: t: move "git add submodule" into test blocks Documentation/config.txt | 3 ++ Documentation/git-add.txt| 7 + advice.c | 2 ++ advice.h | 1 + builtin/add.c| 45 +++- git-submodule.sh | 5 ++-- t/t4041-diff-submodule-option.sh | 8 +++-- t/t4060-diff-submodule-option-diff-format.sh | 8 +++-- t/t7401-submodule-summary.sh | 8 +++-- t/t7414-submodule-mistakes.sh| 40 + 10 files changed, 115 insertions(+), 12 deletions(-) create mode 100755 t/t7414-submodule-mistakes.sh
[PATCH 2/2] t: move "git add submodule" into test blocks
Some submodule tests do some setup outside of a test_expect block. This is bad because we won't actually check the outcome of those commands. But it's doubly so because "git add submodule" now produces a warning to stderr, which is not suppressed by the test scripts in non-verbose mode. This patch does the minimal to fix the annoying warnings. All three of these scripts could use more cleanup of related setup. Signed-off-by: Jeff King --- t/t4041-diff-submodule-option.sh | 8 +--- t/t4060-diff-submodule-option-diff-format.sh | 8 +--- t/t7401-submodule-summary.sh | 8 +--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 2d9731b52..058ee0829 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -430,9 +430,11 @@ test_expect_success 'deleted submodule' ' test_cmp expected actual ' -test_create_repo sm2 && -head7=$(add_file sm2 foo8 foo9) && -git add sm2 +test_expect_success 'create second submodule' ' + test_create_repo sm2 && + head7=$(add_file sm2 foo8 foo9) && + git add sm2 +' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=log HEAD >actual && diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index 33ec26d75..4b168d0ed 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -643,9 +643,11 @@ test_expect_success 'deleted submodule' ' test_cmp expected actual ' -test_create_repo sm2 && -head7=$(add_file sm2 foo8 foo9) && -git add sm2 +test_expect_success 'create second submodule' ' + test_create_repo sm2 && + head7=$(add_file sm2 foo8 foo9) && + git add sm2 +' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=diff HEAD >actual && diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 366746f0d..4e4c45550 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -241,9 +241,11 @@ EOF test_cmp expected actual " -test_create_repo sm2 && -head7=$(add_file sm2 foo8 foo9) && -git add sm2 +test_expect_success 'create second submodule' ' + test_create_repo sm2 && + head7=$(add_file sm2 foo8 foo9) && + git add sm2 +' test_expect_success 'multiple submodules' " git submodule summary >actual && -- 2.13.1.675.g57c06d071
[PATCH 1/2] add: warn when adding an embedded repository
It's an easy mistake to add a repository inside another repository, like: git clone $url git add . The resulting entry is a gitlink, but there's no matching .gitmodules entry. Trying to use "submodule init" (or clone with --recursive) doesn't do anything useful. Prior to v2.13, such an entry caused git-submodule to barf entirely. In v2.13, the entry is considered "inactive" and quietly ignored. Either way, no clone of your repository can do anything useful with the gitlink without the user manually adding the submodule config. In most cases, the user probably meant to either add a real submodule, or they forgot to put the embedded repository in their .gitignore file. Let's issue a warning when we see this case. There are a few things to note: - the warning will go in the git-add porcelain; anybody wanting to do low-level manipulation of the index is welcome to create whatever funny states they want. - we detect the case by looking for a newly added gitlink; updates via "git add submodule" are perfectly reasonable, and this avoids us having to investigate .gitmodules entirely - there's a command-line option to suppress the warning. This is needed for git-submodule itself (which adds the entry before adding any submodule config), but also provides a mechanism for other scripts doing submodule-like things. We could make this a hard error instead of a warning. However, we do add lots of sub-repos in our test suite. It's not _wrong_ to do so. It just creates a state where users may be surprised. Pointing them in the right direction with a gentle hint is probably the best option. There is a config knob that can disable the (long) hint. But I intentionally omitted a config knob to disable the warning entirely. Whether the warning is sensible or not is generally about context, not about the user's preferences. If there's a tool or workflow that adds gitlinks without matching .gitmodules, it should probably be taught about the new command-line option, rather than blanket-disabling the warning. Signed-off-by: Jeff King --- The check for "is this a gitlink" is done by looking for a trailing "/" in the added path. This feels kind of hacky, but actually seems to work well in practice. We've already expanded the pathspecs to real filenames via dir.c, and that omits trees. So anything with a trailing slash must be a gitlink. And I really didn't want to incur any extra cost in the common case here (e.g., checking for "path/.git"). We could do it at zero-cost by pushing the check much further down (i.e., when we'd realize anyway that it's a gitlink), but I didn't want to pollute read-cache.c with what is essentially a porcelain warning. The actual check done there seems to be checking S_ISDIR, but I didn't even want to incur an extra stat per-file. I also waffled on whether we should ask the submodule code whether it knows about a particular path. Technically: git config submodule.foo.path foo git config submodule.foo.url git://... git add foo is legal, but would still warn with this patch. I don't know how much we should care (it would also be easy to do on top). Documentation/config.txt | 3 +++ Documentation/git-add.txt | 7 +++ advice.c | 2 ++ advice.h | 1 + builtin/add.c | 45 ++- git-submodule.sh | 5 +++-- t/t7414-submodule-mistakes.sh | 40 ++ 7 files changed, 100 insertions(+), 3 deletions(-) create mode 100755 t/t7414-submodule-mistakes.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index dd4beec39..e909239bc 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -348,6 +348,9 @@ advice.*:: rmHints:: In case of failure in the output of linkgit:git-rm[1], show directions on how to proceed from the current state. + addEmbeddedRepo:: + Advice on what to do when you've accidentally added one + git repo inside of another. -- core.fileMode:: diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 7ed63dce0..f4169fb1e 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -165,6 +165,13 @@ for "git add --no-all ...", i.e. ignored removed files. be ignored, no matter if they are already present in the work tree or not. +--no-warn-embedded-repo:: + By default, `git add` will warn when adding an embedded + repository to the index without using `git submodule add` to + create an entry in `.gitmodules`. This option will suppress the + warning (e.g., if you are manually performing operations on + submodules). + --chmod=(+|-)x:: Override the executable bit of the added files. The executable bit is only changed in the index, the files on disk are left diff --git a/advice.c b/advi
Hello Beautiful
I hope you are doing well and this email meet you in good health condition. My name is Wesley.I`m from the US but currently in Syria for peace keeping mission. I want to get to know you better, if I may be so bold. I consider myself an easy-going man, and I am currently looking for a relationship in which I feel loved. Please forgive my manners am not good when it comes to Internet because that is not really my field .Here in Syria we are not allowed to go out that makes it very bored for me so I just think I need a friend to talk to outside to keep me going. Please tell me more about yourself, if you don't mind. Hope to hear from you soon. Regards, Wesley.
Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself
On Tue, Jun 13, 2017 at 12:31 AM, René Scharfe wrote: > Am 12.06.2017 um 21:02 schrieb Ævar Arnfjörð Bjarmason: >> >> I only ever use the time offset info to quickly make a mental note of >> "oh +0200, this guy's in Europe", or "oh -0400 America East". Having >> any info at all for %Z would allow me to easily replace that already >> buggy mapping that exists in my head right now with something that's >> at least a bit more accurate, e.g. I remember that +0900 is Japan, but >> I can't now offhand recall what timezones India is at. >> >> Now, obviously we can't know whether to translate e.g. -0400 to New >> York or Santiago, but for the purposes of human readable output I >> thought it would be more useful to guess than show nothing at all. So >> for that purpose the most useful output of %Z *for me* would be just >> showing a string with the 3 most notable cities/areas, weighted for >> showing locations on different continents, e.g.: >> >>+ = Iceland, West Africa, Ittoqqortoormiit >>+0100 = London, Lisbon, Kinshasa >>... >>+0900 = Tokyo, Seul, Manokwari >> >>-0900 = San Francisco, Vancouver, Tijuana >> >>-0600 = Denver, Managua, Calgary >> >> Then I could run: >> >> git log --date=format-local:"%Y[...](%Z)" >> >> And get output like: >> >> commit 826c06412e (HEAD -> master, origin/master, origin/HEAD) >> Author: Junio C Hamano >> Date: Fri Jun 2 15:07:36 2017 +0900 (San Francisco, Vancouver, >> Tijuana etc.) >> >> Fifth batch for 2.14 >> [...] >> commit 30d005c020 >> Author: Jeff King >> Date: Fri May 19 08:59:34 2017 -0400 (New York, Havana, Santiago >> etc.) >> >> diff: use blob path for blob/file diffs >> >> Which gives me a pretty good idea of where the people who are making >> my colleges / collaborators who are making commits all over the world >> are located, for the purposes of reinforcing the abstract numeric >> mapping with a best-guess at what the location might be, or at least >> something that's close to the actual location. > > Half the time this would be off by a zone in areas that use daylight > saving time, or you'd need to determine when DST starts and ends, but > since that depends on the exact time zone it will be tricky. Yes, not to beat this point to death, but to clarify since we still seem to be talking about different things: I'm not saying it isn't fuzzy and inaccurate, I'm saying there's a use-case for human readable output where saying e.g. "somewhere New York-ish" is more useful than nothing, even given summer time I'm still going to know approximately what timezone it is, more so than the offset number. And that is, I would argue, the best thing to do when the user is via %Z asking for a human-readable timezone-ish given a commit object, which only has the offset. > You could use military time zones, which are nice and easy to convert: > Alpha (UTC+1) to Mike (UTC+12) (Juliet is skipped), November (UTC-1) to > Yankee (UTC-12), and Zulu time zone (UTC+0). Downside: Most civilians > don't use them. :) Also there are no names for zones that have an > offset of a fraction of an hour. Yes I think if we're going down the road of providing some human-readable version of the offset this is near-useless since almost nobody using Git is going to know these by heart. >> I'd definitely use a feature like that, and could hack it up on top of >> the current patches if there wasn't vehement opposition to it. Maybe >> the above examples change someone's mind, I can't think of a case >> where someone's relying on %Z now for date-local, or at least cases >> where something like the above wouldn't be more useful in practice >> than just an empty string, but if nobody agrees so be it :) > > Personally I don't have a use for time information; dates alone would > suffice for me -- so I certainly won't hold you back. But I don't see > how it could be done. It could be done by accepting that you're not going to get a perfect solution but one that's good enough to aid the humans currently reading the currently only numeric output. My use-case for this is having colleges all over the world who create commits using different time offsets. It's simply easier to at a glance realize what office someone is in if "git log" is printing city names in/near those timezones, even if none of those cities are where those people are located.
Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself
> On Tue, 13 Jun 2017, René Scharfe wrote: > Am 12.06.2017 um 21:02 schrieb Ævar Arnfjörð Bjarmason: >> Which gives me a pretty good idea of where the people who are making >> my colleges / collaborators who are making commits all over the world >> are located, for the purposes of reinforcing the abstract numeric >> mapping with a best-guess at what the location might be, or at least >> something that's close to the actual location. > Half the time this would be off by a zone in areas that use daylight > saving time, or you'd need to determine when DST starts and ends, but > since that depends on the exact time zone it will be tricky. And sometimes it would be impossible since DST rules differ between hemispheres. For example, there is Europe/Berlin vs Africa/Windhoek, or America/Halifax vs America/Santiago. > You could use military time zones, which are nice and easy to convert: > Alpha (UTC+1) to Mike (UTC+12) (Juliet is skipped), November (UTC-1) to > Yankee (UTC-12), and Zulu time zone (UTC+0). Downside: Most civilians > don't use them. :) Yeah, that would ensure complete confusion. :) You'd have "Quebec" for -0400 whereas the city of Quebec is in zone -0500. Similar for "Lima" denoting +1100. > Also there are no names for zones that have an offset of a fraction > of an hour. There are also zones like Pacific/Tongatapu which have +1300 (and +1400 in summer). All in all I think that Jeff's suggestion makes most sense: Expand %Z to the timezone name for the -local case, when the name is readily available. Otherwise, expand it to the empty string. Ulrich
[PATCH v3] t3200: add test for single parameter passed to -m option
Add a test for the case when only one parameter is passed to '-m'
(move/rename) option.
For example - if 'git branch -m bbb' is run while checked out on aaa
branch, it should rename the currently checked out branch to bbb.
There was no test for this particular case with only one parameter
for -m option. However, there's one similar test case for -M option.
Add test for making sure HEAD points to the bbb (new branch name). Also
add a test for making sure the reflog that is moved to 'bbb' retains
entries created for the currently checked out branch. Note that since
the topmost entry on reflog for bbb will be about branch creation, we
compare bbb@{1} (instead of bbb@{0}) with aaa@{0} to make sure the
reflog for bbb retains entries from aaa.
Signed-off-by: Sahil Dua
---
t/t3200-branch.sh | 17 +
1 file changed, 17 insertions(+)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fe62e7c775da6..8c9cd70696f87 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -100,6 +100,23 @@ test_expect_success 'git branch -m n/n n should work' '
git reflog exists refs/heads/n
'
+# The topmost entry in reflog for branch bbb is about branch creation.
+# Hence, we compare bbb@{1} (instead of bbb@{0}) with aaa@{0}.
+
+test_expect_success 'git branch -m bbb should rename checked out branch' '
+ test_when_finished git branch -D bbb &&
+ test_when_finished git checkout master &&
+ git checkout -b aaa &&
+ git commit --allow-empty -m "a new commit" &&
+ git rev-parse aaa@{0} >expect &&
+ git branch -m bbb &&
+ git rev-parse bbb@{1} >actual &&
+ test_cmp expect actual &&
+ git symbolic-ref HEAD >actual &&
+ echo refs/heads/bbb >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'git branch -m o/o o should fail when o/p exists' '
git branch o/o &&
git branch o/p &&
--
https://github.com/git/git/pull/371
Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed
Hi Junio, On Sat, 10 Jun 2017, Junio C Hamano wrote: > Johannes Schindelin writes: > > > We are about to change the way aliases are expanded, to use the early > > config machinery. > > > > This machinery reports errors in a slightly different manner than the > > cached config machinery. > > > > Let's not get hung up by the precise wording of the message mentioning > > the lin number. It is really sufficient to verify that all the relevant > > information is given to the user. > > > > Signed-off-by: Johannes Schindelin > > --- > > t/t1308-config-set.sh | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh > > index ff50960ccae..69a0aa56d6d 100755 > > --- a/t/t1308-config-set.sh > > +++ b/t/t1308-config-set.sh > > @@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed > > values' ' > > br > > EOF > > test_expect_code 128 git br 2>result && > > - test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result > > + test_i18ngrep "missing value for .alias\.br" result && > > + test_i18ngrep "fatal: .*\.git/config" result && > > + test_i18ngrep "fatal: .*line 2" result > > Just judging from the looks of these, it appears that the updated > messages are more descriptive than the original. Am I mistaken? Sadly, I do not think so. It is just different, not better. Maybe less redundant... See for yourself: Before: error: missing value for 'alias.br' fatal: bad config variable 'alias.br' in file '.git/config' at line 2 After: error: missing value for 'alias.br' fatal: bad config line 2 in file .git/config > If so I think the log message undersells the changes made by the > series by sounding as if you are saying "we are too lazy to bother > giving the same message, so here is a workaround". I fear this is not underselling anything for an improvement. The real fix would indeed be (as mentioned by Brandon elsewhere) to unify the code paths between the cached and the non-cached config machinery, so as to provide the exact same error message in this case. It is annoying that I lack the time to make that happen. My official excuse, though, is that that fix is outside the purpose of this patch series. > In any case, s/lin number/line number/ is needed ;-) Tru. ;-) Cia, Dsch
Re: [PATCH v2 2/8] config: report correct line number upon error
Hi Peff, On Sat, 10 Jun 2017, Jeff King wrote: > On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote: > > > When get_value() parses a key/value pair, it is possible that the line > > number is decreased (because the \n has been consumed already) before the > > key/value pair is passed to the callback function, to allow for the > > correct line to be attributed in case of an error. > > > > However, when git_parse_source() asks get_value() to parse the key/value > > pair, the error reporting is performed *after* get_value() returns. > > > > Which means that we have to be careful not to increase the line number > > in get_value() after the callback function returned an error. > > Good catch. Would we want a test here, like: > > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh > index 13b7851f7..4cad8366a 100755 > --- a/t/t1300-repo-config.sh > +++ b/t/t1300-repo-config.sh > @@ -703,6 +703,12 @@ test_expect_success 'invalid unit' ' > test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in > file .git/config: invalid unit" actual > ' > > +test_expect_success 'invalid path' ' > + echo "[bool]var" >invalid && > + test_must_fail git config -f invalid --path bool.var 2>actual && > + test_i18ngrep "line 1" actual > +' > + > test_expect_success 'invalid stdin config' ' > echo "[broken" | test_must_fail git config --list --file - >output 2>&1 > && > test_i18ngrep "bad config line 1 in standard input" output > > which currently reports "line 2" instead of line 1. Mmmmkay. I am always reluctant to add *even more* stuff to the test suite, in particular since my patch series implicitly changes t1308 to test for this very thing. But I guess my fight against the test suite taking longer and longer and longer is a fight I must lose, as I am pretty alone in that endeavor, and would have have to fight fellow developers. So I guess it is time to give up. I added the test case you suggested, with a title that I find a bit more descriptive. > I wondered if the same bug could be found earlier (e.g,. from > parse_value() when it sees an unpaired quote), but it looks like we do a > cf->linenr-- rollback there already. Right. The underlying bug, of course, is that we use a different machinery to handle cached vs non-cached config, but that is a different story. > > diff --git a/config.c b/config.c > > index 146cb3452ad..9b88531a70d 100644 > > --- a/config.c > > +++ b/config.c > > @@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct > > strbuf *name) > > */ > > cf->linenr--; > > ret = fn(name->buf, value, data); > > - cf->linenr++; > > + if (!ret) > > + cf->linenr++; > > return ret; > > } > > I think this should be "if (ret < 0)". The caller only considers it an > error if get_value() returns a negative number. As you have it here, I > think a config callback which returned a positive number would end up > with nonsense line numbers. I think you are half-correct: it should be `if (ret >= 0)` (the linenr needs to be modified back in case of success, not in case of failure, in case of failure there will be some reporting going on that needs the same line number as `fn()` had seen). Ciao, Dscho
Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory
Hi Peff,
On Sat, 10 Jun 2017, Jeff King wrote:
> On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:
>
> > -char *alias_lookup(const char *alias)
> > [...]
> > {
> > - char *v = NULL;
> > - struct strbuf key = STRBUF_INIT;
> > - strbuf_addf(&key, "alias.%s", alias);
> > - if (git_config_key_is_valid(key.buf))
> > - git_config_get_string(key.buf, &v);
> > - strbuf_release(&key);
> > - return v;
> > + struct strbuf key;
> > + char *v;
> > +};
> > [...]
> > +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)
> > +{
> > + struct config_alias_data data = { STRBUF_INIT, NULL };
> > +
> > + strbuf_addf(&data.key, "alias.%s", alias);
> > + if (git_config_key_is_valid(data.key.buf))
> > + read_early_config(config_alias_cb, &data, cdup_dir);
> > + strbuf_release(&data.key);
> > +
> > + return data.v;
> > }
>
> Two optional cleanups here:
>
> 1. We don't need to call config_key_is_valid when using a callback. We
> only needed that to prevent the configset machinery from issuing a
> warning. It does save us reading the config entirely when the
> program name is syntactically invalid as an alias, but that's a
> pretty rare case.
It may be a pretty rare case, or it may not be. I do not want to think
hard about this, so I just wanted to keep that test.
But since you suggested it, I will simply blame all the fallout (if any)
on you.
;-)
> 2. Now that we're not using the configset machinery, we don't need to
> have the alias name as a full string. Instead of using the strbuf,
> we could just pass the "alias" string itself and do:
>
>if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
>
> in the callback.
As you probably guessed, I had tried that first and then figured that if
I needed to keep the config_key_is_valid() test anyway, I could just as
well keep the strbuf around for later use.
Will change the code,
Dscho
Re: [PATCH v2 8/8] Use the early config machinery to expand aliases
Hi Peff,
On Sat, 10 Jun 2017, Jeff King wrote:
> On Thu, Jun 08, 2017 at 09:53:53PM +0200, Johannes Schindelin wrote:
>
> > @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int
> > *argc, int *envchanged)
> >
> > static int handle_alias(int *argcp, const char ***argv)
> > {
> > + struct strbuf cdup_dir = STRBUF_INIT;
> > int envchanged = 0, ret = 0, saved_errno = errno;
> > int count, option_count;
> > const char **new_argv;
> > const char *alias_command;
> > char *alias_string;
> > - int unused_nongit;
> > -
> > - save_env_before_alias();
> > - setup_git_directory_gently(&unused_nongit);
> >
> > alias_command = (*argv)[0];
> > - alias_string = alias_lookup(alias_command, NULL);
> > + alias_string = alias_lookup(alias_command, &cdup_dir);
> > if (alias_string) {
> > if (alias_string[0] == '!') {
> > struct child_process child = CHILD_PROCESS_INIT;
> >
> > + if (cdup_dir.len)
> > + setup_git_directory();
> > +
>
> I'm really confused here. We went to all the trouble to record the
> cdup_dir as part of the alias lookup so that we could make sure to
> chdir() to it.
Right, that was my first idea.
But then the test suite taught me that we set at least GIT_PREFIX and that
tests break (or, in other words, promises Git made earlier) if those
variables are not set.
So as a quick fix, I simply called setup_git_directory() instead of
chdir(cdup_dir.buf).
> I understand why you must use setup_git_directory() and not just a
> chdir(), because the latter sets up variables like GIT_PREFIX and
> GIT_DIR that the shell alias may be depending on.
Exactly.
> But couldn't we just unconditionally do:
>
> setup_git_directory_gently();
But of course we can do that! Why on earth did I not think of that...
Will change the code accordingly.
Ciao,
Dscho
Re: [PATCH v2 2/8] config: report correct line number upon error
On Tue, Jun 13, 2017 at 01:02:49PM +0200, Johannes Schindelin wrote: > > +test_expect_success 'invalid path' ' > > + echo "[bool]var" >invalid && > > + test_must_fail git config -f invalid --path bool.var 2>actual && > > + test_i18ngrep "line 1" actual > > +' > > + > > test_expect_success 'invalid stdin config' ' > > echo "[broken" | test_must_fail git config --list --file - >output 2>&1 > > && > > test_i18ngrep "bad config line 1 in standard input" output > > > > which currently reports "line 2" instead of line 1. > > Mmmmkay. > > I am always reluctant to add *even more* stuff to the test suite, in > particular since my patch series implicitly changes t1308 to test for this > very thing. If there's another test that covers this, I'm happy to use that. I just didn't notice it. > > > + if (!ret) > > > + cf->linenr++; > > > return ret; > > > } > > > > I think this should be "if (ret < 0)". The caller only considers it an > > error if get_value() returns a negative number. As you have it here, I > > think a config callback which returned a positive number would end up > > with nonsense line numbers. > > I think you are half-correct: it should be `if (ret >= 0)` (the linenr > needs to be modified back in case of success, not in case of failure, in > case of failure there will be some reporting going on that needs the same > line number as `fn()` had seen). Oops, right. -Peff
Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory
On Tue, Jun 13, 2017 at 01:21:28PM +0200, Johannes Schindelin wrote: > > Two optional cleanups here: > > > > 1. We don't need to call config_key_is_valid when using a callback. We > > only needed that to prevent the configset machinery from issuing a > > warning. It does save us reading the config entirely when the > > program name is syntactically invalid as an alias, but that's a > > pretty rare case. > > It may be a pretty rare case, or it may not be. I do not want to think > hard about this, so I just wanted to keep that test. > > But since you suggested it, I will simply blame all the fallout (if any) > on you. > > ;-) I accept all the blame. :) (And I was the person who added the is_valid check here, so I can take both blame and credit for the whole thing). -Peff
Re: [PATCH v2 8/8] Use the early config machinery to expand aliases
On Tue, Jun 13, 2017 at 01:25:53PM +0200, Johannes Schindelin wrote: > > But couldn't we just unconditionally do: > > > > setup_git_directory_gently(); > > But of course we can do that! Why on earth did I not think of that... > > Will change the code accordingly. Thanks. I was really worried you were going to come back with some subtle point that I missed. But I'm happy we will be able to take the simpler route. -Peff
[PATCH] git-stash: fix pushing stash with pathspec from subdir
The `git stash push` command recently gained the ability to get a
pathspec as its argument to only stash matching files. Calling this
command from a subdirectory does not work, though, as one of the first
things we do is changing to the top level directory without keeping
track of the prefix from which the command is being run.
Fix the shortcoming by storing the prefix previous to the call to
`cd_to_toplevel` and then subsequently using `git rev-parse --prefix` to
correctly resolve the pathspec. Add a test to catch future breakage of
this usecase.
Signed-off-by: Patrick Steinhardt
---
git-stash.sh | 3 +++
t/t3903-stash.sh | 16
2 files changed, 19 insertions(+)
diff --git a/git-stash.sh b/git-stash.sh
index 2fb651b2b..e7b85932d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -19,6 +19,7 @@ OPTIONS_SPEC=
START_DIR=$(pwd)
. git-sh-setup
require_work_tree
+prefix=$(git rev-parse --show-prefix) || exit 1
cd_to_toplevel
TMP="$GIT_DIR/.git-stash.$$"
@@ -273,6 +274,8 @@ push_stash () {
shift
done
+ eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
+
if test -n "$patch_mode" && test -n "$untracked"
then
die "$(gettext "Can't use --patch and --include-untracked or
--all at the same time")"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3b4bed5c9..4046817d7 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -812,6 +812,22 @@ test_expect_success 'stash -- stashes and
restores the file' '
test_path_is_file bar
'
+test_expect_success 'stash -- stashes in subdirectory' '
+ mkdir sub &&
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ (
+ cd sub &&
+ git stash push -- ../foo
+ ) &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_expect_success 'stash with multiple pathspec arguments' '
>foo &&
>bar &&
--
2.13.1
Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory
Hi Peff,
On Tue, 13 Jun 2017, Johannes Schindelin wrote:
> On Sat, 10 Jun 2017, Jeff King wrote:
>
> > On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:
> >
> > > -char *alias_lookup(const char *alias)
> > > [...]
> > > {
> > > - char *v = NULL;
> > > - struct strbuf key = STRBUF_INIT;
> > > - strbuf_addf(&key, "alias.%s", alias);
> > > - if (git_config_key_is_valid(key.buf))
> > > - git_config_get_string(key.buf, &v);
> > > - strbuf_release(&key);
> > > - return v;
> > > + struct strbuf key;
> > > + char *v;
> > > +};
> > > [...]
> > > +char *alias_lookup(const char *alias, struct strbuf *cdup_dir)
> > > +{
> > > + struct config_alias_data data = { STRBUF_INIT, NULL };
> > > +
> > > + strbuf_addf(&data.key, "alias.%s", alias);
> > > + if (git_config_key_is_valid(data.key.buf))
> > > + read_early_config(config_alias_cb, &data, cdup_dir);
> > > + strbuf_release(&data.key);
> > > +
> > > + return data.v;
> > > }
> >
> > Two optional cleanups here:
> >
> > 1. We don't need to call config_key_is_valid when using a callback. We
> > only needed that to prevent the configset machinery from issuing a
> > warning. It does save us reading the config entirely when the
> > program name is syntactically invalid as an alias, but that's a
> > pretty rare case.
>
> It may be a pretty rare case, or it may not be. I do not want to think
> hard about this, so I just wanted to keep that test.
>
> But since you suggested it, I will simply blame all the fallout (if any)
> on you.
>
> ;-)
>
> > 2. Now that we're not using the configset machinery, we don't need to
> > have the alias name as a full string. Instead of using the strbuf,
> > we could just pass the "alias" string itself and do:
> >
> >if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> >
> > in the callback.
>
> As you probably guessed, I had tried that first and then figured that if
> I needed to keep the config_key_is_valid() test anyway, I could just as
> well keep the strbuf around for later use.
>
> Will change the code,
Alas, I won't change the code after all.
It is really tempting to avoid the extra strbuf, but then the error
message would change from
error: missing value for 'alias.br'
to
error: missing value for 'br'
which is of course no good at all.
And since I already have to keep that strbuf, I'll simply keep the
config_key_is_valid() guard, too (because why not).
Ciao,
Dscho
Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory
On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote: > > As you probably guessed, I had tried that first and then figured that if > > I needed to keep the config_key_is_valid() test anyway, I could just as > > well keep the strbuf around for later use. > > > > Will change the code, > > Alas, I won't change the code after all. > > It is really tempting to avoid the extra strbuf, but then the error > message would change from > > error: missing value for 'alias.br' > > to > > error: missing value for 'br' > > which is of course no good at all. > > And since I already have to keep that strbuf, I'll simply keep the > config_key_is_valid() guard, too (because why not). Oof, yeah, that is definitely worse. I'm fine with keeping both parts. -Peff
[PATCH v3 0/6] Avoid problem where git_dir is set after alias expansion
When expanding an alias in a subdirectory, we setup the git_dir (gently), read the config, and then restore the "env" (e.g. the current working directory) so that the command specified by the alias can run correctly. What we failed to reset was the git_dir, meaning that in the most common case, it was now pointing to a .git/ directory *in the subdirectory*. This problem was identified in the GVFS fork, where a pre-command hook was introduced to allow pre-fetching missing blobs. An early quick fix in the GVFS fork simply built on top of the save_env_before_alias() hack, introducing another hack that saves the git_dir and restores it after an alias is expanded: https://github.com/Microsoft/git/commit/2d859ba3b That is very hacky, though, and it is much better (although much more involved, too) to fix this "properly", i.e. by replacing the ugly save/restore logic by simply using the early config code path. However, aliases are strange beasts. When an alias refers to a single Git command (originally the sole intention of aliases), the current working directory is restored to what it had been before expanding the alias. But when an alias starts with an exclamation point, i.e. referring to a command-line to be interpreted by the shell, the current working directory is no longer in the subdirectory but instead in the worktree's top-level directory. This is even true for worktrees added by `git worktree add`. But when we are inside the .git/ directory, the current working directory is *restored* to the subdirectory inside the .git/ directory. In short, the logic is a bit complicated what is the expected current working directory after expanding an alias and before actually running it. That is why this patch series had to expand the signature of the early config machinery to return the additional information for aliases' benefit. Changes since v2: - fixed tyop in the commit message where "line number" was lacking the first "e" - added a test for the "report line number" fix, and test the `ret` variable to be non-negative (instead of zero) - dropped 'read_early_config(): optionally return the worktree's top-level directory' as well as most parts of 'alias_lookup(): optionally return top-level directory', as we have to run setup_git_directory() in the shell alias code path anyway Johannes Schindelin (6): discover_git_directory(): avoid setting invalid git_dir config: report correct line number upon error help: use early config when autocorrecting aliases t1308: relax the test verifying that empty alias values are disallowed t7006: demonstrate a problem with aliases in subdirectories Use the early config machinery to expand aliases alias.c| 31 +--- config.c | 3 ++- git.c | 55 -- help.c | 2 +- setup.c| 1 + t/t1300-repo-config.sh | 6 ++ t/t1308-config-set.sh | 4 +++- t/t7006-pager.sh | 11 ++ 8 files changed, 52 insertions(+), 61 deletions(-) base-commit: 41dd4330a1210003bd702ec4a9301ed68e60864d Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v3 Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v3 Interdiff vs v2: $(git diff $rebasedtag..$branchname) -- 2.13.0.windows.1.460.g13f583bedb5
[PATCH v3 1/6] discover_git_directory(): avoid setting invalid git_dir
When discovering a .git/ directory, we take pains to ensure that its
repository format version matches Git's expectations, and we return NULL
otherwise.
However, we still appended the invalid path to the strbuf passed as
argument.
Let's just reset the strbuf to the state before we appended the .git/
directory that was eventually rejected.
There is another early return path in that function, when
setup_git_directory_gently_1() returns GIT_DIR_NONE or an error. In that
case, the gitdir parameter has not been touched, therefore there is no
need for an equivalent change in that code path.
Signed-off-by: Johannes Schindelin
---
setup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/setup.c b/setup.c
index e3f7699a902..2435186e448 100644
--- a/setup.c
+++ b/setup.c
@@ -982,6 +982,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
warning("ignoring git dir '%s': %s",
gitdir->buf + gitdir_offset, err.buf);
strbuf_release(&err);
+ strbuf_setlen(gitdir, gitdir_offset);
return NULL;
}
--
2.13.0.windows.1.460.g13f583bedb5
[PATCH v3 3/6] help: use early config when autocorrecting aliases
Git has this feature which suggests similar commands (including aliases)
in case the user specified an unknown command.
This feature currently relies on a side effect of the way we expand
aliases right now: when a command is not a builtin, we use the regular
config machinery (meaning: discovering the .git/ directory and
initializing global state such as the config cache) to see whether the
command refers to an alias.
However, we will change the way aliases are expanded in the next
commits, to use the early config instead. That means that the
autocorrect feature can no longer discover the available aliases by
looking at the config cache (because it has not yet been initialized).
So let's just use the early config machinery instead.
This is slightly less performant than the previous way, as the early
config is used *twice*: once to see whether the command refers to an
alias, and then to see what aliases are most similar. However, this is
hardly a performance-critical code path, so performance is less important
here.
Signed-off-by: Johannes Schindelin
---
help.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/help.c b/help.c
index db7f3d79a01..b44c55ec2da 100644
--- a/help.c
+++ b/help.c
@@ -289,7 +289,7 @@ const char *help_unknown_cmd(const char *cmd)
memset(&other_cmds, 0, sizeof(other_cmds));
memset(&aliases, 0, sizeof(aliases));
- git_config(git_unknown_cmd_config, NULL);
+ read_early_config(git_unknown_cmd_config, NULL);
load_command_list("git-", &main_cmds, &other_cmds);
--
2.13.0.windows.1.460.g13f583bedb5
[PATCH v3 5/6] t7006: demonstrate a problem with aliases in subdirectories
When expanding aliases, the git_dir is set during the alias expansion (by virtue of running setup_git_directory_gently()). This git_dir may be relative to the current working directory, and indeed often is simply ".git/". When the alias expands to a shell command, we restore the original working directory, though, yet we do not reset git_dir. As a consequence, subsequent read_early_config() runs will mistake the git_dir to be populated properly and not find the correct config. Demonstrate this problem by adding a test case. Signed-off-by: Johannes Schindelin --- t/t7006-pager.sh | 11 +++ 1 file changed, 11 insertions(+) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 4f3794d415e..83881ec3a0c 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -391,6 +391,17 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' ' ) ' +test_expect_failure TTY 'core.pager is found via alias in subdirectory' ' + sane_unset GIT_PAGER && + test_config core.pager "cat >via-alias" && + ( + cd sub && + rm -f via-alias && + test_terminal git -c alias.r="-p rev-parse" r HEAD && + test_path_is_file via-alias + ) +' + test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense' test_pager_choices 'git shortlog' -- 2.13.0.windows.1.460.g13f583bedb5
[PATCH v3 6/6] Use the early config machinery to expand aliases
Instead of discovering the .git/ directory, read the config and then
trying to painstakingly reset all the global state if we did not find a
matching alias, let's use the early config machinery instead.
It may look like unnecessary work to discover the .git/ directory in the
early config machinery and then call setup_git_directory_gently() in the
case of a shell alias, repeating the very same discovery *again*.
However, we have to do this as the early config machinery takes pains
*not* to touch any global state, while shell aliases expect a possibly
changed working directory and at least the GIT_PREFIX and GIT_DIR
variables to be set.
Also, one might be tempted to streamline the code in alias_lookup() to
*not* use a strbuf for the key. However, if the config reports an error,
it is far superior to tell the user that the `alias.xyz` key had a
problem than to claim that it was the `xyz` key.
This change also fixes a known issue where Git tried to read the pager
config from an incorrect path in a subdirectory of a Git worktree if an
alias expanded to a shell command.
Signed-off-by: Johannes Schindelin
---
alias.c | 31 ---
git.c| 55 ---
t/t7006-pager.sh | 2 +-
3 files changed, 29 insertions(+), 59 deletions(-)
diff --git a/alias.c b/alias.c
index 3b90397a99d..6bdc9363037 100644
--- a/alias.c
+++ b/alias.c
@@ -1,14 +1,31 @@
#include "cache.h"
+struct config_alias_data
+{
+ struct strbuf key;
+ char *v;
+};
+
+static int config_alias_cb(const char *key, const char *value, void *d)
+{
+ struct config_alias_data *data = d;
+
+ if (!strcmp(key, data->key.buf))
+ return git_config_string((const char **)&data->v, key, value);
+
+ return 0;
+}
+
char *alias_lookup(const char *alias)
{
- char *v = NULL;
- struct strbuf key = STRBUF_INIT;
- strbuf_addf(&key, "alias.%s", alias);
- if (git_config_key_is_valid(key.buf))
- git_config_get_string(key.buf, &v);
- strbuf_release(&key);
- return v;
+ struct config_alias_data data = { STRBUF_INIT, NULL };
+
+ strbuf_addf(&data.key, "alias.%s", alias);
+ if (git_config_key_is_valid(data.key.buf))
+ read_early_config(config_alias_cb, &data);
+ strbuf_release(&data.key);
+
+ return data.v;
}
#define SPLIT_CMDLINE_BAD_ENDING 1
diff --git a/git.c b/git.c
index 8ff44f081d4..58ef570294d 100644
--- a/git.c
+++ b/git.c
@@ -16,50 +16,6 @@ const char git_more_info_string[] =
"to read about a specific subcommand or concept.");
static int use_pager = -1;
-static char *orig_cwd;
-static const char *env_names[] = {
- GIT_DIR_ENVIRONMENT,
- GIT_WORK_TREE_ENVIRONMENT,
- GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
- GIT_PREFIX_ENVIRONMENT
-};
-static char *orig_env[4];
-static int save_restore_env_balance;
-
-static void save_env_before_alias(void)
-{
- int i;
-
- assert(save_restore_env_balance == 0);
- save_restore_env_balance = 1;
- orig_cwd = xgetcwd();
- for (i = 0; i < ARRAY_SIZE(env_names); i++) {
- orig_env[i] = getenv(env_names[i]);
- orig_env[i] = xstrdup_or_null(orig_env[i]);
- }
-}
-
-static void restore_env(int external_alias)
-{
- int i;
-
- assert(save_restore_env_balance == 1);
- save_restore_env_balance = 0;
- if (!external_alias && orig_cwd && chdir(orig_cwd))
- die_errno("could not move to %s", orig_cwd);
- free(orig_cwd);
- for (i = 0; i < ARRAY_SIZE(env_names); i++) {
- if (external_alias &&
- !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
- continue;
- if (orig_env[i]) {
- setenv(env_names[i], orig_env[i], 1);
- free(orig_env[i]);
- } else {
- unsetenv(env_names[i]);
- }
- }
-}
static void commit_pager_choice(void) {
switch (use_pager) {
@@ -250,19 +206,18 @@ static int handle_alias(int *argcp, const char ***argv)
const char **new_argv;
const char *alias_command;
char *alias_string;
- int unused_nongit;
-
- save_env_before_alias();
- setup_git_directory_gently(&unused_nongit);
alias_command = (*argv)[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
if (alias_string[0] == '!') {
struct child_process child = CHILD_PROCESS_INIT;
+ int nongit_ok;
+
+ /* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */
+ setup_git_directory_gently(&nongit_ok);
commit_pager_choice();
- restore_env(1);
child.use_shell = 1;
argv_array_push(&
[PATCH v3 2/6] config: report correct line number upon error
When get_value() parses a key/value pair, it is possible that the line number is decreased (because the \n has been consumed already) before the key/value pair is passed to the callback function, to allow for the correct line to be attributed in case of an error. However, when git_parse_source() asks get_value() to parse the key/value pair, the error reporting is performed *after* get_value() returns. Which means that we have to be careful not to increase the line number in get_value() after the callback function returned an error. Signed-off-by: Johannes Schindelin --- config.c | 3 ++- t/t1300-repo-config.sh | 6 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 146cb3452ad..12c172e298a 100644 --- a/config.c +++ b/config.c @@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name) */ cf->linenr--; ret = fn(name->buf, value, data); - cf->linenr++; + if (ret >= 0) + cf->linenr++; return ret; } diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 13b7851f7c2..a37ef042221 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -703,6 +703,12 @@ test_expect_success 'invalid unit' ' test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual ' +test_expect_success 'line number is reported correctly' ' + printf "[bool]\n\tvar\n" >invalid && + test_must_fail git config -f invalid --path bool.var 2>actual && + test_i18ngrep "line 2" actual +' + test_expect_success 'invalid stdin config' ' echo "[broken" | test_must_fail git config --list --file - >output 2>&1 && test_i18ngrep "bad config line 1 in standard input" output -- 2.13.0.windows.1.460.g13f583bedb5
[PATCH v3 4/6] t1308: relax the test verifying that empty alias values are disallowed
We are about to change the way aliases are expanded, to use the early config machinery. This machinery reports errors in a slightly different manner than the cached config machinery. Let's not get hung up by the precise wording of the message mentioning the line number. It is really sufficient to verify that all the relevant information is given to the user. Signed-off-by: Johannes Schindelin --- t/t1308-config-set.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index ff50960ccae..69a0aa56d6d 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed values' ' br EOF test_expect_code 128 git br 2>result && - test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result + test_i18ngrep "missing value for .alias\.br" result && + test_i18ngrep "fatal: .*\.git/config" result && + test_i18ngrep "fatal: .*line 2" result ' test_expect_success 'error on modifying repo config without repo' ' -- 2.13.0.windows.1.460.g13f583bedb5
Re: Feature Request: Show status of the stash in git status command
Hi, On 13/06/17 02:42 AM, Konstantin Khomoutov wrote: > On Mon, Jun 12, 2017 at 11:42:44PM -0400, liam Beguin wrote: > > [...] >>> Conceptually, the contents of the stash are *not* commits, even >>> though the implementation happens to use a commit to represent each >>> stash entry. Perhaps "has %d entry/entries" is an improvement, but >>> a quick scanning of an early part of "git stash --help" tells me >>> that >> >> what's different between a stash and a commit? > > The same that exists between an interface and a concrete implementation > in a programming language. Makes sense, I thought there was a more fundamental difference. > > "A stash entry" is a concept which is defined to keep explicitly > recorded untracked files and which can be applied, shown and deleted > from the stash bag (well, you can create a branch off it as well). I've noticed this but I don't understand when it can be used. I'll try to find out more on this. > > The fact a stash entry is a merge commit of two synthetic commits is an > implementation detail. It can be very useful at times for power users, > but regular Git users need not be concerned with this. > > Another fact worth reiterating that what the UI displays to the user is > better to match what the user reads in the docs. ;-) > I'll make changes as suggested by Junio. I slightly prefer "Your stash has %d entry/entries" over "You have %d stash/stashes" but I'll go with what's used elsewhere in the documentation. Thanks, - Liam
Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed
Johannes Schindelin writes: > Sadly, I do not think so. It is just different, not better. Maybe less > redundant... See for yourself: Yup, I noticed and was referring to this "less redundant" as an improvement, actually. > The real fix would indeed be (as mentioned by Brandon elsewhere) to unify > the code paths between the cached and the non-cached config machinery, so > as to provide the exact same error message in this case. Yeah, the unifying of the messages would be a good addition in the mid term but I tend to agree that it can be done after this series lands. Thanks for clarification.
Re: Feature Request: Show status of the stash in git status command
liam Beguin writes: >> The fact a stash entry is a merge commit of two synthetic commits is an >> implementation detail. It can be very useful at times for power users, >> but regular Git users need not be concerned with this. >> >> Another fact worth reiterating that what the UI displays to the user is >> better to match what the user reads in the docs. ;-) > > I'll make changes as suggested by Junio. I slightly prefer > "Your stash has %d entry/entries" over "You have %d stash/stashes" > but I'll go with what's used elsewhere in the documentation. Yup, I agree that I would definitely call them "stash entries" if I were writing the documentation today, but lets match the new message to the existing lingo first and think about renaming "a stash" to "a stash entry" as a separate step. The latter would become a larger change (we'd also need to add an entry to glossary-contents.txt). Thanks.
Re: [PATCH v2] doc: fix location of index in worktree scenatio
Torsten Bögershausen writes: > That's better ... here is my attempt to improve That reads well. Thanks. > > doc: do not use `rm .git/index` when normalizing line endings > > When illustrating how to normalize the line endings, the > documentation in gitattributes tells the user to `rm .git/index`. > > This is incorrect for two reasons. Users shouldn't be instructed > to futz with the internal implementation of Git using raw > file system tools like "rm". > Second, when submodules or second working trees are used, ".git" > is a not a directory but a "gitfile" pointing at the location of the > real ".git" directory, `rm .git/index` does not work. > > The point of the step in the illustration is to remove all > entries from the index without touching the working tree, and > the way to do it with Git is to use `read-tree --empty`.
Attension my friend.
Attension my friend, Your silent is not helping us on this, please let me know your response concerning my first letter to you. Lawson.
[ANNOUNCE] Git for Windows 2.13.1
Dear Git users, It is my pleasure to announce that Git for Windows 2.13.1 is available from: https://git-for-windows.github.io/ Changes since Git for Windows v2.13.0 (May 10th 2017) New Features * Comes with Git v2.13.1. * Comes with Git Credential Manager v1.10.0. * Comes with OpenSSH 7.5p1. * Comes with Git Flow v1.11.0. * Comes with Git LFS v2.1.1. * Git now uses the flag introduced with Windows 10 Creators Update to create symbolic links without requiring elevated privileges in Developer Mode. Bug Fixes * The documentation of Git for Windows' several config files was improved. * When interrupting Git processes in Git Bash by pressing Ctrl+C, Git now removes .lock files as designed (accompanying Git PR; this should also fix issue #338). * git status -uno now treats submodules in ignored directories correctly. * The fscache feature no longer slows down git commit -m in large worktrees. * Executing git.exe in Git Bash when the current working directory is a UNC path now works as expected. * Staging/unstaging multiple files in Git GUI via Ctrl+C now works. * When hitting Ctrl+T in Git GUI to stage files, but the file list is empty, Git GUI no longer shows an exception window. Filename | SHA-256 | --- Git-2.13.1-64-bit.exe | fe834ec34b6cbda5b973fb4a210998471451efaa42ffe20d6b5de197a95ffc13 Git-2.13.1-32-bit.exe | 6b8f7605eafb982efcec53f128cedaa2535d589207b368cade61ce7ca5f04b26 PortableGit-2.13.1-64-bit.7z.exe | f47957cf596019ace07ef3fc17d08591f0e85092f4ca760850b6f34cabba95ba PortableGit-2.13.1-32-bit.7z.exe | 8468716d1c32f22394b17534d8346cf352ddce1cecaf6df985d2338106350242 MinGit-2.13.1-64-bit.zip | 4e361db36ebec015797499c197c4e994070cfa76f80efa283c3eef89e9d1ae81 MinGit-2.13.1-32-bit.zip | 8985d57a4410103db339719e9143f367f5645a4ee74d72246a74253fb4ede70b Git-2.13.1-64-bit.tar.bz2 | a87df3c348d32c91d3f8f76bbe3b621339880b659a5c904bce10c7c96626d756 Git-2.13.1-32-bit.tar.bz2 | 2efd7a1049fcdadfd7ee23fc41bdbf61cf4b185eac706028418991d5882f56a9 Ciao, Johannes
Re: proposal for how to share other refs as part of refs/tracking/*
On 2017-06-12 06:58 PM, Jacob Keller wrote: Hi, There's no actual code yet, (forgive me), but I've been thinking back to a while ago about attempting to find a way to share things like refs/notes, and similar refs which are usually not shared across a remote. By default, those refs are not propagated when you do a push or a pull, and this makes using them in any project which has more then one repository quite difficult. I'm going to focus the discussion primarily on refs/notes as this is what I am most interested in, but I think similar issues exist for refs/grafts and refs/replace, and maybe other sources? More formal support for custom ref namespaces would be a boon. For example, we have our nightly builds create a "build/w.x.y-z" ref (we only want to tag official releases). Sharing those refs is not hard, but a bit obscure. For branches, we already have a system to share the status of remote branches, called "refs/remotes//" This hierarchy unfortunately does not keep space for non-branches, because you can't simply add a "refs/remotes/notes/<>" or "refs/remotes//notes" to this as it would be ambiguous. Now, you might just decide to push the refs/notes directly, ie: git push origin refs/notes/...:refs/notes/... Unfortunately, this has problems because it leaves no standard way to distinguish your local work from what is on the remote, so you can't easily merge the remote work into yours. There was a related discussion in the run-up to 1.8.0, about a "remote tag namespace" to support having different remotes with the same tag name for different objects. See these messages and their surrounding threads: http://public-inbox.org/git/AANLkTikeqsg+qJ0z4iQ6ZmKL=_HB8YX_z20L=dffa...@mail.gmail.com/ http://public-inbox.org/git/AANLkTi=yfwoaqmhhvlsb1_xmyoe9hhp2yb4h4tqzw...@mail.gmail.com/ http://public-inbox.org/git/201102020322.00171.jo...@herland.net/ The discussion explored, among other things, making refs/remotes/$remote/* a mirror of a remote's own refs/* hierarchy (well, within reason -- I think there are limits to what should be mirrored). So I like your refs/tracking proposal, and hope that it aims for mirroring a remote's refs, to eventually replace refs/remotes entirely. M. For example, if Alice creates a new note and pushes it, then Bob creates a different note on the same commit, he needs to be able to merge Alice's changes into his note, just like one would do when two people commit to the same branch. Today, he must pull the remote notes into a separate location, (since pulling directly into refs/notes will overwrite his work), and then update, and then push. Because this is not standardized, it becomes unwieldy to actually perform on a day to day basis. I propose that we add a new "refs/tracking" hierarchy which will be used to track these type of refs We could even (long term) migrate refs/remotes into refs/tracking instead, or provide both with the refs/remotes being pointers or something like that.. Essentially, refs/notes would be pulled into refs/tracking//notes/* or something along these lines. Then, git notes would be modified to be able to have commands to "pull" and "push" notes which would fetch the remote, and then attempt a merge into the local notes, while a push would update the remote. I chose "tracking" because it sort of fits the concept and does not include things like "remote-notes" or "remotes-v2" which are a bit weird. I'm posting this on the mailing list prior to having code because I wanted to get a sense of how people felt about the question and whether others still felt it was an issue. Essentially the goal is to standardize how any refs namespace can be shared in such a way that local copies can tell what the remote had at a fetch time, in order to allow easier handling of conflicts between local and remote changes, just like we do for branches (heads) but generalized so that other refs namespaces can get the same ability to handle conflicts. Thanks, Jake
Re: [PATCH 4/4] config: don't implicitly use gitdir
On 06/13, Jeff King wrote:
> On Mon, Jun 12, 2017 at 11:16:27PM -0700, Brandon Williams wrote:
>
> > > > *puzzled* Why wasn't this needed before, then? The rest of the patch
> > > > should result in no functional change, but this part seems different.
> > >
> > > Now I'm puzzled, too. The original that got filled in lazily by the
> > > config functions was always get_git_dir(). I can buy the argument that
> > > this was a bug (I'm not familiar enough with worktree to say one way or
> > > the other), but if it's a fix it should definitely go into another
> > > patch.
> >
> > Well actually... in do_git_config_sequence 'git_path("config")' is
> > called which will convert gitdir to commondir under the hood. you can't
> > use vanilla gitdir because the config isn't stored in a worktree's
> > gitdir but rather in the commondir as the config is shared by all
> > worktrees.
>
> Sorry, I missed the fact that there were two sites changed on the first
> read.
Well I missed that fact when I first wrote these patches too :)
>
> > So maybe we actually need to add a field to the 'config_options' struct
> > of 'commondir' such that the commondir can be used to load the actual
> > config file and 'gitdir' can be used to handle the 'IncludeIf' stuff.
>
> On reflection, I suspect that probably is the case. If you have a
> workdir in ~/foo, you probably want to match IncludeIf against that
> instead of wherever the common dir happens to be.
K I'll look into adding that then. I will say keeping track of
'commondir' vs 'gitdir' does get slightly confusing.
--
Brandon Williams
Re: [PATCH 4/4] config: don't implicitly use gitdir
On 06/13, Jeff King wrote:
> On Mon, Jun 12, 2017 at 10:52:43PM -0700, Brandon Williams wrote:
>
> > > >> curious: Why get_git_common_dir() instead of get_git_dir()?
> > > >
> > > > Needs to be commondir since the config is stored in the common git
> > > > directory and not a per worktree git directory.
> > >
> > > *puzzled* Why wasn't this needed before, then? The rest of the patch
> > > should result in no functional change, but this part seems different.
> >
> > there is no functional change, this is what always happened.
> > git_path("config") will replace gitdir with commondir under the hood.
>
> Of the two callsites you removed, one is git_pathdup(), and the other
> is get_git_dir(). So they weren't matched, though I suspect the one in
> include_by_gitdir probably ought to have been commondir?
Good point, I hope my reply on the other part of the thread answers
this.
--
Brandon Williams
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes: > When using git-blame lots of lines contain redundant information, for > example in hunks that consist of multiple lines, the metadata (commit name, > author, timezone) are repeated. A reader may not be interested in those, > so darken them. The darkening is not just based on hunk, but actually > takes the previous lines content for that field to compare to. > > Signed-off-by: Stefan Beller > --- Not about "blame", but I was trying the --color-moved stuff on Brandon's "create config.h" patch and found its behaviour somewhat different from what I recall we discussed. I thought that the adjacentbounds mode was invented to dim (i.e. not attract undue attention) to most of the moved lines, but highlight only the boundary of moved blocks, so I expected most of the new and old lines in that patch would be shown in the "context" color, except for the boundary between two blocks of removed lines that have gone to different location (and similarly two blocks of new lines that have come from different location) would be painted in oldmoved and newmoved colors and their alternatives. Instead I see all old and new lines that are moved painted in these colors, without any dimming. Is my expectation off?
Re: [PATCH] git-stash: fix pushing stash with pathspec from subdir
Patrick Steinhardt writes:
> The `git stash push` command recently gained the ability to get a
> pathspec as its argument to only stash matching files. Calling this
> command from a subdirectory does not work, though, as one of the first
> things we do is changing to the top level directory without keeping
> track of the prefix from which the command is being run.
>
> Fix the shortcoming by storing the prefix previous to the call to
> `cd_to_toplevel` and then subsequently using `git rev-parse --prefix` to
> correctly resolve the pathspec. Add a test to catch future breakage of
> this usecase.
It sounds more like a simple bug than "shortcoming" ;-), and the
right approach to fix it is to add the original prefix before the
pathspecs before using them, which is exactly what your patch does.
Looks good.
I suspect that "rev-parse --prefix" needs a bit of tweak to make it
truly usable for pathspecs with magic, but that is a totally
separate issue.
Thanks.
> Signed-off-by: Patrick Steinhardt
> ---
> git-stash.sh | 3 +++
> t/t3903-stash.sh | 16
> 2 files changed, 19 insertions(+)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 2fb651b2b..e7b85932d 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -19,6 +19,7 @@ OPTIONS_SPEC=
> START_DIR=$(pwd)
> . git-sh-setup
> require_work_tree
> +prefix=$(git rev-parse --show-prefix) || exit 1
> cd_to_toplevel
>
> TMP="$GIT_DIR/.git-stash.$$"
> @@ -273,6 +274,8 @@ push_stash () {
> shift
> done
>
> + eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> +
> if test -n "$patch_mode" && test -n "$untracked"
> then
> die "$(gettext "Can't use --patch and --include-untracked or
> --all at the same time")"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9..4046817d7 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -812,6 +812,22 @@ test_expect_success 'stash -- stashes and
> restores the file' '
> test_path_is_file bar
> '
>
> +test_expect_success 'stash -- stashes in subdirectory' '
> + mkdir sub &&
> + >foo &&
> + >bar &&
> + git add foo bar &&
> + (
> + cd sub &&
> + git stash push -- ../foo
> + ) &&
> + test_path_is_file bar &&
> + test_path_is_missing foo &&
> + git stash pop &&
> + test_path_is_file foo &&
> + test_path_is_file bar
> +'
> +
> test_expect_success 'stash with multiple pathspec arguments' '
> >foo &&
> >bar &&
Re: proposal for how to share other refs as part of refs/tracking/*
On 2017-06-13 10:41 AM, Marc Branchaud wrote: So I like your refs/tracking proposal, and hope that it aims for mirroring a remote's refs, to eventually replace refs/remotes entirely. To be extra-clear: I think a refs/tracking hierarchy that starts with notes and maybe some other bits is a good first step. I *hope* such a first step can eventually mirror all of a remote's refs, including heads and tags. I in no way think that this effort should begin by tackling heads and tags. M.
[PATCH 1/3] config: create a function to format section headers
Factor out the logic which creates section headers in the config file,
e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.
This introduces no function changes, but is needed for a later change
which adds support for copying branch sections in the config file.
Signed-off-by: Sahil Dua
Signed-off-by: Ævar Arnfjörð Bjarmason
---
config.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 146cb3452adab..d5bb69e925dac 100644
--- a/config.c
+++ b/config.c
@@ -2169,10 +2169,10 @@ static int write_error(const char *filename)
return 4;
}
-static int store_write_section(int fd, const char *key)
+struct strbuf store_create_section(const char *key)
{
const char *dot;
- int i, success;
+ int i;
struct strbuf sb = STRBUF_INIT;
dot = memchr(key, '.', store.baselen);
@@ -2188,6 +2188,15 @@ static int store_write_section(int fd, const char *key)
strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
}
+ return sb;
+}
+
+static int store_write_section(int fd, const char *key)
+{
+ int success;
+
+ struct strbuf sb = store_create_section(key);
+
success = write_in_full(fd, sb.buf, sb.len) == sb.len;
strbuf_release(&sb);
--
https://github.com/git/git/pull/363
[PATCH 2/3] branch: add test for -m renaming multiple config sections
From: Ævar Arnfjörð Bjarmason Add a test for how 'git branch -m' handles the renaming of multiple config sections existing for one branch. The config format we use is hybrid machine/human editable, and we do our best to preserve the likes of comments and formatting when editing the file with git-config. This adds a test for the currently expected semantics in the face of some rather obscure edge cases which are unlikely to occur in practice. Helped-by: Sahil Dua Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Sahil Dua --- t/t3200-branch.sh | 37 + 1 file changed, 37 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 10f8f026ffb4b..28c02ffeadb4f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -341,6 +341,43 @@ test_expect_success 'config information was renamed, too' ' test_must_fail git config branch.s/s.dummy ' +test_expect_success 'git branch -m correctly renames multiple config sections' ' + test_when_finished "git checkout master" && + git checkout -b source master && + + # Assert that a config file with multiple config sections has + # those sections preserved... + cat >expect <<-\EOF && + branch.dest.key1=value1 + some.gar.b=age + branch.dest.key2=value2 + EOF + cat >config.branch <<\EOF && +;; Comment for source +[branch "source"] + ;; Comment for the source value + key1 = value1 + ;; Comment for some.gar +[some "gar"] + ;; Comment for the some.gar value + b = age + ;; Comment for source, again +[branch "source"] + ;; Comment for the source value, again + key2 = value2 +EOF + cat config.branch >>.git/config && + git branch -m source dest && + git config -f .git/config -l | grep -F -e source -e dest -e some.gar >actual && + test_cmp expect actual && + + # ...and that the comments for those sections are also + # preserved. + cat config.branch | sed "s/\"source\"/\"dest\"/" >expect && + grep -A 9001 "Comment for source" .git/config >actual && + test_cmp expect actual +' + test_expect_success 'deleting a symref' ' git branch target && git symbolic-ref refs/heads/symref refs/heads/target && -- https://github.com/git/git/pull/363
[PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)
Add the ability to --copy a branch and its reflog and configuration,
this uses the same underlying machinery as the --move (-m) option
except the reflog and configuration is copied instead of being moved.
This is useful for e.g. copying a topic branch to a new version,
e.g. work to work-2 after submitting the work topic to the list, while
preserving all the tracking info and other configuration that goes
with the branch, and unlike --move keeping the other already-submitted
branch around for reference.
Like --move, when the source branch is the currently checked out
branch the HEAD is moved to the destination branch. In the case of
--move we don't really have a choice (other than remaining on a
detached HEAD), but it makes sense to do the same for --copy.
The most common usage of this feature is expected to be moving to a
new topic branch which is a copy of the current one, in that case
moving to the target branch is what the user wants, and doesn't
unexpectedly behave differently than --move would.
One outstanding caveat of this implementation is that:
git checkout maint &&
git checkout master &&
git branch -c topic &&
git checkout -
Will check out 'maint' instead of 'master'. This is because the @{-N}
feature (or its -1 shorthand "-") relies on HEAD reflogs created by
the checkout command, so in this case we'll checkout maint instead of
master, as the user might expect. What to do about that is left to a
future change.
Helped-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Sahil Dua
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/git-branch.txt | 14 ++-
builtin/branch.c | 67 ++
cache.h | 2 +
config.c | 102 +
refs.c | 11 +++
refs.h | 9 +-
refs/files-backend.c | 46 --
refs/refs-internal.h | 4 +
t/t3200-branch.sh| 211 +++
9 files changed, 420 insertions(+), 46 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b7741f..94fd89ddc5ab9 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -18,6 +18,7 @@ SYNOPSIS
'git branch' (--set-upstream-to= | -u ) []
'git branch' --unset-upstream []
'git branch' (-m | -M) []
+'git branch' (-c | -C) []
'git branch' (-d | -D) [-r] ...
'git branch' --edit-description []
@@ -64,6 +65,10 @@ If had a corresponding reflog, it is renamed to
match
renaming. If exists, -M must be used to force the rename
to happen.
+The `-c` and `-C` options have the exact same semantics as `-m` and
+`-M`, except instead of the branch being renamed it along with its
+config and reflog will be copied to a new name.
+
With a `-d` or `-D` option, `` will be deleted. You may
specify more than one branch for deletion. If the branch currently
has a reflog then the reflog will also be deleted.
@@ -104,7 +109,7 @@ OPTIONS
In combination with `-d` (or `--delete`), allow deleting the
branch irrespective of its merged status. In combination with
`-m` (or `--move`), allow renaming the branch even if the new
- branch name already exists.
+ branch name already exists, the same applies for `-c` (or `--copy`).
-m::
--move::
@@ -113,6 +118,13 @@ OPTIONS
-M::
Shortcut for `--move --force`.
+-c::
+--copy::
+ Copy a branch and the corresponding reflog.
+
+-C::
+ Shortcut for `--copy --force`.
+
--color[=]::
Color branches to highlight current, local, and
remote-tracking branches.
diff --git a/builtin/branch.c b/builtin/branch.c
index 83fcda43dceec..89f64f4123a5e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -27,6 +27,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [] [-l] [-f] []"),
N_("git branch [] [-r] (-d | -D) ..."),
N_("git branch [] (-m | -M) [] "),
+ N_("git branch [] (-c | -C) [] "),
N_("git branch [] [-r | -a] [--points-at]"),
N_("git branch [] [-r | -a] [--format]"),
NULL
@@ -449,15 +450,19 @@ static void reject_rebase_or_bisect_branch(const char
*target)
free_worktrees(worktrees);
}
-static void rename_branch(const char *oldname, const char *newname, int force)
+static void copy_or_rename_branch(const char *oldname, const char *newname,
int copy, int force)
{
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg =
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
int clobber_head_ok;
- if (!oldname)
- die(_("cannot rename the current branch while not on any."));
+ if (!oldname) {
+ if (copy)
+ die(_("cannot copy the current branch while not on
any."));
+ else
+ die(_("cannot rename the current b
Re: [PATCH v3 6/6] Use the early config machinery to expand aliases
Johannes Schindelin writes: > Instead of discovering the .git/ directory, read the config and then > trying to painstakingly reset all the global state if we did not find a > matching alias, let's use the early config machinery instead. s/read/&ing/, I think. My reading hiccupped while trying to figure out what two alternative approaches are being compared. > It may look like unnecessary work to discover the .git/ directory in the > early config machinery and then call setup_git_directory_gently() in the > case of a shell alias, repeating the very same discovery *again*. > However, we have to do this as the early config machinery takes pains > *not* to touch any global state, while shell aliases expect a possibly > changed working directory and at least the GIT_PREFIX and GIT_DIR > variables to be set. Makes sense. Nicely explained. > Also, one might be tempted to streamline the code in alias_lookup() to > *not* use a strbuf for the key. However, if the config reports an error, > it is far superior to tell the user that the `alias.xyz` key had a > problem than to claim that it was the `xyz` key. The mention of "streamline" is puzzling to me. When we are trying "git xyz", "alias.xyz" is the key we would look up, not "xyz"; it is not clear to anybody who didn't read the discussion on v2 (which includes the readers of "git log" in a few months) what kind of flawed streamlining could look up "xyz" and result in a bad configuration reported on it. > alias.c | 31 --- > git.c| 55 --- > t/t7006-pager.sh | 2 +- > 3 files changed, 29 insertions(+), 59 deletions(-) Happy to see the deletion of all the save/restore-env stuff. Except for the puzzlement in one paragraph in the log, looks very good. Thanks for a pleasant reading.
Re: [RFC/PATCH] builtin/blame: darken redundant line information
On Tue, Jun 13, 2017 at 8:25 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author, timezone) are repeated. A reader may not be interested in those,
>> so darken them. The darkening is not just based on hunk, but actually
>> takes the previous lines content for that field to compare to.
>>
>> Signed-off-by: Stefan Beller
>> ---
>
> Not about "blame", but I was trying the --color-moved stuff on
> Brandon's "create config.h" patch and found its behaviour somewhat
> different from what I recall we discussed.
We discussed several things and we did not come to an agreement,
what is best, so I implemented different things there.
> I thought that the
> adjacentbounds mode was invented to dim (i.e. not attract undue
> attention) to most of the moved lines, but highlight only the
> boundary of moved blocks,
I agree up to this part. And when you use the standard color scheme,
the new/old moved is dimmed according to my perception of colors.
If you use an individual setup for colors, you need to adjust the four
color.diff.{old, new}Moved[Alternative] slots.
> so I expected most of the new and old
> lines in that patch would be shown in the "context" color,
So instead of a special color in this mode you expected "context"
for color.diff.{old, new}Moved and "highlighted" for the alternative slots.
> except
> for the boundary between two blocks of removed lines that have gone
> to different location (and similarly two blocks of new lines that
> have come from different location) would be painted in oldmoved and
> newmoved colors and their alternatives. Instead I see all old and
> new lines that are moved painted in these colors, without any
> dimming.
http://i.imgur.com/36DMRBl.png is what I see (regular colors,
"git show --color-moved f2f1da5348ff2f297b43b") and
that is what I would expect.
>
> Is my expectation off?
Maybe? It sounds as if you expected 'context' color to be used
in the adjacentbounds.
* Do you expect 'context' to be used in other modes as well?
* Do you expect this as code/algorithm change or would rather
a change of default colors (default {old/new}Moved = 'context')
be sufficient to meet that expectation?
* As you have an individual color setup, maybe you can fix this
for you by setting the appropriate slots to your perception of
dimmed?
Thanks,
Stefan
Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory
Jeff King writes: > On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote: > >> > As you probably guessed, I had tried that first and then figured that if >> > I needed to keep the config_key_is_valid() test anyway, I could just as >> > well keep the strbuf around for later use. >> > >> > Will change the code, >> >> Alas, I won't change the code after all. >> >> It is really tempting to avoid the extra strbuf, but then the error >> message would change from >> >> error: missing value for 'alias.br' >> >> to >> >> error: missing value for 'br' >> >> which is of course no good at all. >> >> And since I already have to keep that strbuf, I'll simply keep the >> config_key_is_valid() guard, too (because why not). > > Oof, yeah, that is definitely worse. I'm fine with keeping both parts. When you replace Dscho's "compare 'var' with 'alias.br' that is in strbuf naively with the "skip-prefix and compare with br" without changing anything else, i.e. if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key)) return git_config_string((const char **)&data->v, key, value); it would cause the "br" to be fed to git_config_string() and result in problem reported for "br", not "alias.br". But this can be trivially fixed by passing "var" instead of "key" to git_config_string(), no? Am I mistaken?
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes:
> * As you have an individual color setup, maybe you can fix this
> for you by setting the appropriate slots to your perception of
> dimmed?
I do not think it is possible with only {new,old}{,alternative} 4
colors.
Consider this diff:
context
-B
-B
-B
-A
-A
-A
context
+A
+A
+A
+B
+B
+B
context
Two blocks (A and B) that are adjacent are moved but swapped to form
a pair of new adjacent blocks.
We would like the boundary between the last "-B" and the first "-A"
to be highlighted differently; all other "-A" and "-B" lines do not
disappear but go elsewhere, so they want to be dimmed.
The newly added 6 lines are actually moved from elsewhere, and we
would like the boundary between the last "+A" and the first "+B" to
be highlighted differently, and others are dimmed.
So I'd think you would need at least two kinds of highlight colors
plus a dimmed color.
context
-B dim
-B dim
-B highlight
-A highlight
-A dim
-A dim
context
+A dim
+A dim
+A highlight
+B highlight
+B dim
+B dim
context
If old_moved and old_moved_alternative are meant for highlighting
"-B" and "-A" above differently, while new_moved and
new_moved_alternative are for highlighting "+A" and "+B"
differently, you'd need a way to specify "dim" for old and new moved
lines, which seems to be impossible with only 4 new colors.
Re: [PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)
Sahil Dua writes: > Add the ability to --copy a branch and its reflog and configuration, > this uses the same underlying machinery as the --move (-m) option > except the reflog and configuration is copied instead of being moved. > > This is useful for e.g. copying a topic branch to a new version, > e.g. work to work-2 after submitting the work topic to the list, while > preserving all the tracking info and other configuration that goes > with the branch, and unlike --move keeping the other already-submitted > branch around for reference. > > Like --move, when the source branch is the currently checked out > branch the HEAD is moved to the destination branch. In the case of > --move we don't really have a choice (other than remaining on a > detached HEAD), but it makes sense to do the same for --copy. I strongly disagree with this "it makes sense to do the same". It would equally (if not more) make sense to keep the HEAD pointing at the same. Personally, I may use this feature if it didn't move HEAD, but I wouldn't if HEAD gets moved. But that may be just me.
Re: [PATCH 1/3] config: create a function to format section headers
Sahil Dua writes:
> Factor out the logic which creates section headers in the config file,
> e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.
>
> This introduces no function changes, but is needed for a later change
> which adds support for copying branch sections in the config file.
>
> Signed-off-by: Sahil Dua
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> config.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 146cb3452adab..d5bb69e925dac 100644
> --- a/config.c
> +++ b/config.c
> @@ -2169,10 +2169,10 @@ static int write_error(const char *filename)
> return 4;
> }
>
> -static int store_write_section(int fd, const char *key)
> +struct strbuf store_create_section(const char *key)
> {
> const char *dot;
> - int i, success;
> + int i;
> struct strbuf sb = STRBUF_INIT;
>
> dot = memchr(key, '.', store.baselen);
> @@ -2188,6 +2188,15 @@ static int store_write_section(int fd, const char *key)
> strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
> }
>
> + return sb;
> +}
> +
> +static int store_write_section(int fd, const char *key)
> +{
> + int success;
> +
> + struct strbuf sb = store_create_section(key);
> +
> success = write_in_full(fd, sb.buf, sb.len) == sb.len;
> strbuf_release(&sb);
>
>
> --
> https://github.com/git/git/pull/363
Makes sense.
Re: [PATCH 4/4] config: don't implicitly use gitdir
Jeff King wrote: > On Mon, Jun 12, 2017 at 06:38:17PM -0700, Jonathan Nieder wrote: >> Brandon Williams wrote: >>> On 06/12, Jonathan Nieder wrote: Alternatively, could this patch rename git_config_with_options? That way any other patch in flight that calls git_config_with_options would conflict with this patch, giving us an opportunity to make sure it also sets git_dir. As another nice side benefit it would make it easy for someone reading the patch to verify it didn't miss any callers. >> [...] >>> And I don't know if I agree with renaming a function just to rename it. >> >> I forgot to say: another way to accomplish the same thing can be to >> reorder the function's arguments. The relevant thing is to make code >> that calls the function without being aware of the new requirements >> fail to compile. > > If the parameter is now required, then it might make sense for it to > become an actual function parameter instead of being stuffed into the > config_options struct. That would give you your breaking change, plus > make it more obvious to the reader that it is not optional. I like it. I also like that in your proposed patch the no longer necessary local git_dir goes away. What's the next step? The discussion of get_git_common_dir() reveals that switching from git_path() to mkpathdup() loses the adjust_git_path() step and everything becomes complicated. So perhaps adding opts.git_common_dir and using that instead of git_path should happen as a preliminary patch, making this patch afterward a more straightforward refactoring. Thanks, Jonathan
Re: [PATCH 1/2] add: warn when adding an embedded repository
On Tue, Jun 13, 2017 at 2:24 AM, Jeff King wrote: > It's an easy mistake to add a repository inside another > repository, like: > > git clone $url > git add . > > The resulting entry is a gitlink, but there's no matching > .gitmodules entry. Trying to use "submodule init" (or clone > with --recursive) doesn't do anything useful. Prior to > v2.13, such an entry caused git-submodule to barf entirely. > In v2.13, the entry is considered "inactive" and quietly > ignored. Either way, no clone of your repository can do > anything useful with the gitlink without the user manually > adding the submodule config. > > In most cases, the user probably meant to either add a real > submodule, or they forgot to put the embedded repository in > their .gitignore file. > > Let's issue a warning when we see this case. There are a few > things to note: > > - the warning will go in the git-add porcelain; anybody > wanting to do low-level manipulation of the index is > welcome to create whatever funny states they want. > > - we detect the case by looking for a newly added gitlink; > updates via "git add submodule" are perfectly reasonable, > and this avoids us having to investigate .gitmodules > entirely > > - there's a command-line option to suppress the warning. > This is needed for git-submodule itself (which adds the > entry before adding any submodule config), but also > provides a mechanism for other scripts doing > submodule-like things. > > We could make this a hard error instead of a warning. > However, we do add lots of sub-repos in our test suite. It's > not _wrong_ to do so. It just creates a state where users > may be surprised. Pointing them in the right direction with > a gentle hint is probably the best option. Sounds good up to here (and right). > There is a config knob that can disable the (long) hint. But > I intentionally omitted a config knob to disable the warning > entirely. Whether the warning is sensible or not is > generally about context, not about the user's preferences. > If there's a tool or workflow that adds gitlinks without > matching .gitmodules, it should probably be taught about the > new command-line option, rather than blanket-disabling the > warning. > > Signed-off-by: Jeff King > --- > The check for "is this a gitlink" is done by looking for a > trailing "/" in the added path. This feels kind of hacky, > but actually seems to work well in practice. This whole "slash at the end" thing comes from extensive use of shell completion adding the slash at the end of a directory IMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is the same underlying hack.) > We've already > expanded the pathspecs to real filenames via dir.c, and that > omits trees. So anything with a trailing slash must be a > gitlink. Oh! > > And I really didn't want to incur any extra cost in the > common case here (e.g., checking for "path/.git"). We could > do it at zero-cost by pushing the check much further down > (i.e., when we'd realize anyway that it's a gitlink), but I > didn't want to pollute read-cache.c with what is essentially > a porcelain warning. The actual check done there seems to be > checking S_ISDIR, but I didn't even want to incur an extra > stat per-file. makes sense. > > I also waffled on whether we should ask the submodule code > whether it knows about a particular path. Technically: > > git config submodule.foo.path foo > git config submodule.foo.url git://... > git add foo > > is legal, but would still warn with this patch. I don't know > how much we should care (it would also be easy to do on > top). And here I was thinking this is not legal, because you may override anything *except* submodule.*.path in the config. That is because all the other settings (such as url, active flag, branch, shallow recommendation) are dependent on the use case, the user, changes to the environment (url) or such. The name<->path mapping however is only to be changed via changes to the tracked content. That is why it would make sense to disallow overriding the path outside the tracked content. In my ideal dream world of submodules we would have the following: $ cat .gitmodules [submodule "sub42"] path = foo # path only in tree! $ cat .git/config ... [submodule] active = . active = :(exclude)Irrelevant/submodules/for/my/usecase/* # note how this is user centric $ git show refs/meta/magic/for/refs/heads/master:.gitmodules [submodule "sub42"] url = https://example.org/foo branch = . # Note how this is neither centering on the in-tree # contents, nor the user. Instead it focuses on the # project or group. It is *workflow* centric. # Workflows may change over time, e.g. the url could # be repointed to k.org or an in-house mirror without tree # changes. But back to reviewing this patch. > > Documentation/config.txt | 3 +++ > Documentation/git-add.txt | 7 +++ > advice.c | 2 ++
Re: [PATCH 1/3] config: create a function to format section headers
On Tue, Jun 13, 2017 at 6:17 PM, Sahil Dua wrote: > Factor out the logic which creates section headers in the config file, > e.g. the 'branch.foo' key will be turned into '[branch "foo"]'. +CC Junio: This is to be applied, Sahil is using submitgit which apparently doesn't CC you by default (I don't know if there's some option for that he missed). Also +CC Lars who hacked up -m initially, Alex who hacked on some of the logic of git_config_rename_section(), and Johannes who also hacked on some of the guts of that code. Also, in the interest of full disclosure. Sahil's a co-worker of mine who I've been mentoring on how to submit changes to git.git, after I added a note to the internal company announcement saying I'd upgraded git advertising that I'd be happy to help anyone who's interested contribute to the project. That explains the off-list review & Sahil submitting a patch of mine first seen as part of this series.
Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections
Sahil Dua writes: > + cat >expect <<-\EOF && > + branch.dest.key1=value1 > + some.gar.b=age > + branch.dest.key2=value2 > + EOF > + cat >config.branch <<\EOF && > +;; Comment for source > +[branch "source"] > + ;; Comment for the source value > + key1 = value1 > + ;; Comment for some.gar > +[some "gar"] > + ;; Comment for the some.gar value > + b = age > + ;; Comment for source, again > +[branch "source"] > + ;; Comment for the source value, again > + key2 = value2 > +EOF Indenting using <<- would make it easier to read. I.e. cat >config.branch <<-\EOF && ;; Comment for ... [branch "source"] ;; Comment for ... ... EOF > + cat config.branch >>.git/config && > + git branch -m source dest && > + git config -f .git/config -l | grep -F -e source -e dest -e some.gar > >actual && > + test_cmp expect actual && > + > + # ...and that the comments for those sections are also > + # preserved. > + cat config.branch | sed "s/\"source\"/\"dest\"/" >expect && > + grep -A 9001 "Comment for source" .git/config >actual && Where does 9001 come from? Is that just "an arbitrary large number"? Besides, "grep -A" is quite unportable. Would sed -n -e "/Comment for source/,$p" .git/config >actual work equally well? > + test_cmp expect actual > +' > + > test_expect_success 'deleting a symref' ' > git branch target && > git symbolic-ref refs/heads/symref refs/heads/target && > > -- > https://github.com/git/git/pull/363
Re: [RFC/PATCH] builtin/blame: darken redundant line information
On Tue, Jun 13, 2017 at 10:00 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> * As you have an individual color setup, maybe you can fix this
>> for you by setting the appropriate slots to your perception of
>> dimmed?
>
> I do not think it is possible with only {new,old}{,alternative} 4
> colors.
>
> Consider this diff:
>
> context
> -B
> -B
> -B
> -A
> -A
> -A
> context
> +A
> +A
> +A
> +B
> +B
> +B
> context
>
> Two blocks (A and B) that are adjacent are moved but swapped to form
> a pair of new adjacent blocks.
>
> We would like the boundary between the last "-B" and the first "-A"
> to be highlighted differently; all other "-A" and "-B" lines do not
> disappear but go elsewhere, so they want to be dimmed.
>
> The newly added 6 lines are actually moved from elsewhere, and we
> would like the boundary between the last "+A" and the first "+B" to
> be highlighted differently, and others are dimmed.
>
> So I'd think you would need at least two kinds of highlight colors
> plus a dimmed color.
Here is what currently happens:
>
> context
> -B dim oldMoved
> -B dim oldMoved
> -B highlight oldMovedAlternative
> -A highlight oldMovedAlternative
> -A dim oldMoved
> -A dim oldMoved
> context
> +A dim newMoved
> +A dim newMoved
> +A highlight newMovedAlternative
> +B highlight newMovedAlternative
> +B dim newMoved
> +B dim newMoved
> context
>
So the there is only one "highlight" color in each block.
There is no separate hightligh-for-ending-block and
highlight-for-new-block respectively.
> If old_moved and old_moved_alternative are meant for highlighting
> "-B" and "-A" above differently, while new_moved and
> new_moved_alternative are for highlighting "+A" and "+B"
> differently, you'd need a way to specify "dim" for old and new moved
> lines, which seems to be impossible with only 4 new colors.
The standard non alternative was dim in my mind for the
adjacentbounds mode.
If you prefer to have the alternate mode, you rather want no dim,
no highlight, but just 2 alternating colors of equal attention-drawing.
Re: [PATCH 2/2] t: move "git add submodule" into test blocks
On Tue, Jun 13, 2017 at 2:24 AM, Jeff King wrote: > Some submodule tests do some setup outside of a test_expect > block. This is bad because we won't actually check the > outcome of those commands. But it's doubly so because "git > add submodule" now produces a warning to stderr, which is > not suppressed by the test scripts in non-verbose mode. Makes sense. > This patch does the minimal to fix the annoying warnings. > All three of these scripts could use more cleanup of related > setup. agreed. > > Signed-off-by: Jeff King Reviewed-by: Stefan Beller
Re: [PATCH 1/2] add: warn when adding an embedded repository
On 06/13, Stefan Beller wrote: > On Tue, Jun 13, 2017 at 2:24 AM, Jeff King wrote: > > > There is a config knob that can disable the (long) hint. But > > I intentionally omitted a config knob to disable the warning > > entirely. Whether the warning is sensible or not is > > generally about context, not about the user's preferences. > > If there's a tool or workflow that adds gitlinks without > > matching .gitmodules, it should probably be taught about the > > new command-line option, rather than blanket-disabling the > > warning. > > > > Signed-off-by: Jeff King > > --- > > The check for "is this a gitlink" is done by looking for a > > trailing "/" in the added path. This feels kind of hacky, > > but actually seems to work well in practice. > > This whole "slash at the end" thing comes from extensive use > of shell completion adding the slash at the end of a directory > IMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is > the same underlying hack.) I got rid of PATHSPEC_STRIP_SUBMODULE_SLASH_* recently, just an fyi. -- Brandon Williams
Re: [PATCH 1/2] add: warn when adding an embedded repository
Jeff King writes: > I also waffled on whether we should ask the submodule code > whether it knows about a particular path. Technically: > > git config submodule.foo.path foo > git config submodule.foo.url git://... > git add foo > > is legal, but would still warn with this patch. Did you mean "git config -f .gitmodules" for the first two? If so, I think I agree that (1) it should be legal and (2) we should be able to add the check on top of this patch. Or do you really mean the case in which these are in .git/config?
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes: > Here is what currently happens: > >> >> context >> -B dim oldMoved >> -B dim oldMoved >> -B highlight oldMovedAlternative >> -A highlight oldMovedAlternative >> -A dim oldMoved >> -A dim oldMoved >> context >> +A dim newMoved >> +A dim newMoved >> +A highlight newMovedAlternative >> +B highlight newMovedAlternative >> +B dim newMoved >> +B dim newMoved >> context >> > > So the there is only one "highlight" color in each block. > There is no separate hightligh-for-ending-block and > highlight-for-new-block respectively. I think the adjacentbounds mode is simply broken if that is the design. In the above simplified case, you can get away with only a single "highlight" color, but you cannot tell where the boundaries are when three or more lines are shuffled, no?
Re: [PATCH] send-email: Add tocmd option to suppress-cc
Viresh Kumar writes:
>> Going back to the core part of your change, i.e.
>>
>> -foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
>> +foreach my $entry (qw (tocmd cccmd cc author self sob body bodycc)) {
>>
>> to think about it a bit more, I notice that all the existing ones
>> are about CC. So I am not just not convinced that tocmd belongs to
>> the same class. I am inclined to say that it smells quite different
>> from others.
>
> That's right but how do we solve this problem then?
>
> Add another optional argument like suppress-to ? I thought that it
> would be better to override suppress-cc rather than attempting any
> such thing.
>
> I am fine with any solution that address the concerns raised by this
> patch though.
I am not sure what problem is being solved, quite honestly. "I have
tocmd configured and I want a way not to specify tocmd for a single
invocation?" Would something along the lines of
git -c sendemail.tocmd=true send-email ...
how you do it in general? Do you want a prettier-looking
equivalent, e.g.
git send-email --tocmd=true ...
or something like that?
Re: [RFC/PATCH] builtin/blame: darken redundant line information
On Tue, Jun 13, 2017 at 10:19 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> Here is what currently happens: >> >>> >>> context >>> -B dim oldMoved >>> -B dim oldMoved >>> -B highlight oldMovedAlternative >>> -A highlight oldMovedAlternative >>> -A dim oldMoved >>> -A dim oldMoved >>> context >>> +A dim newMoved >>> +A dim newMoved >>> +A highlight newMovedAlternative >>> +B highlight newMovedAlternative >>> +B dim newMoved >>> +B dim newMoved >>> context >>> >> >> So the there is only one "highlight" color in each block. >> There is no separate hightligh-for-ending-block and >> highlight-for-new-block respectively. > > I think the adjacentbounds mode is simply broken if that is the > design. ok. Going by this reasoning, would you claim that allbounds would also be broken design: > git show --color-moved=allbounds: > context > -B oldMovedAlternative > -B oldMoved > -B oldMovedAlternative > -A oldMovedAlternative > -A oldMoved > -A oldMovedAlternative > context > +A newMovedAlternative > +A newMoved > +A newMovedAlternative > +B newMovedAlternative > +B newMoved > +B newMovedAlternative > context > > In the above simplified case, you can get away with only a single > "highlight" color, but you cannot tell where the boundaries are when > three or more lines are shuffled, no? But you do not want to (yet)? The goal is not to tell you where the bounds are, but the goal is to point out that extra care is required for review of these particular 3 lines. So IMHO this feature helps for drawing reviewer attention, but not for explaining blocks. In an extreme alternative design, we would have just annotated each hunk in the context lines for example telling that there are n out of m new lines. But that information by itself is not useful for review Instead this alternative moved line detection could have an impact on diff stats.
Re: [PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)
Junio C Hamano writes: > Sahil Dua writes: > >> Add the ability to --copy a branch and its reflog and configuration, >> this uses the same underlying machinery as the --move (-m) option >> except the reflog and configuration is copied instead of being moved. >> >> This is useful for e.g. copying a topic branch to a new version, >> e.g. work to work-2 after submitting the work topic to the list, while >> preserving all the tracking info and other configuration that goes >> with the branch, and unlike --move keeping the other already-submitted >> branch around for reference. >> >> Like --move, when the source branch is the currently checked out >> branch the HEAD is moved to the destination branch. In the case of >> --move we don't really have a choice (other than remaining on a >> detached HEAD), but it makes sense to do the same for --copy. > > I strongly disagree with this "it makes sense to do the same". It > would equally (if not more) make sense to keep the HEAD pointing at > the same. > > Personally, I may use this feature if it didn't move HEAD, but I > wouldn't if HEAD gets moved. But that may be just me. Ah, that came out to be stronger than I intended. While I do prefer "the HEAD is not moved by this command---if you want to move to the newly created branch after copying, check it out yourself" a lot better than what the patch does, I do not think I'd care so strongly that I'd reject this patch series unless the behaviour is changed. But I do react strongly to an unsubstantiated claim "it makes sense to do the same". I can buy "We anticipate that in 50% of the case users would find this branch switching annoying and in the other 50% of the case, users would find it useful; since we need to pick one, we just randomly decide to do the same as --move", though.
Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections
On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano wrote: > Sahil Dua writes: > >> + cat >expect <<-\EOF && >> + branch.dest.key1=value1 >> + some.gar.b=age >> + branch.dest.key2=value2 >> + EOF >> + cat >config.branch <<\EOF && >> +;; Comment for source >> +[branch "source"] >> + ;; Comment for the source value >> + key1 = value1 >> + ;; Comment for some.gar >> +[some "gar"] >> + ;; Comment for the some.gar value >> + b = age >> + ;; Comment for source, again >> +[branch "source"] >> + ;; Comment for the source value, again >> + key2 = value2 >> +EOF > > Indenting using <<- would make it easier to read. I.e. > > cat >config.branch <<-\EOF && > ;; Comment for ... > [branch "source"] > ;; Comment for ... > ... > EOF I should have added a comment for that, I can't find any portable (but suggestions welcome) way to do that and preserve the indentation, so the test_cmp would still succeed if the moving/renaming function munged all leading whitespace in the config with -\EOF as opposed to \EOF. >> + cat config.branch >>.git/config && >> + git branch -m source dest && >> + git config -f .git/config -l | grep -F -e source -e dest -e some.gar >> >actual && >> + test_cmp expect actual && >> + >> + # ...and that the comments for those sections are also >> + # preserved. >> + cat config.branch | sed "s/\"source\"/\"dest\"/" >expect && >> + grep -A 9001 "Comment for source" .git/config >actual && > > Where does 9001 come from? Is that just "an arbitrary large > number"? > > Besides, "grep -A" is quite unportable. Would > > sed -n -e "/Comment for source/,$p" .git/config >actual > > work equally well? It's just a sufficiently large number, I thought -A was portable enough after grepping the test suite, but on closer inspection it turns out those were all git-grep invocations, oops. Yeah all I need here is all lines after a line matching a given string, so that sed command works, will fix it up to use that. >> + test_cmp expect actual >> +' >> + >> test_expect_success 'deleting a symref' ' >> git branch target && >> git symbolic-ref refs/heads/symref refs/heads/target && >> >> -- >> https://github.com/git/git/pull/363
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes: > But you do not want to (yet)? The goal is not to tell you where the bounds > are, but the goal is to point out that extra care is required for review of > these particular 3 lines. And when you _can_ help users in that "extra care" by pointing out where the boundary is, what is the justification for hiding that information?
Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections
Ævar Arnfjörð Bjarmason writes: > On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano wrote: >> Indenting using <<- would make it easier to read. I.e. >> >> cat >config.branch <<-\EOF && >> ;; Comment for ... >> [branch "source"] >> ;; Comment for ... >> ... >> EOF > > I should have added a comment for that, I can't find any portable (but > suggestions welcome) way to do that and preserve the indentation, so > the test_cmp would still succeed if the moving/renaming function > munged all leading whitespace in the config with -\EOF as opposed to > \EOF. Ah, I see why it is done that way. You could indent the lines in the configuration file with SPs (<<- strips only HTs, no?) > It's just a sufficiently large number, I thought -A was portable > enough after grepping the test suite, but on closer inspection it > turns out those were all git-grep invocations, oops. Yeah all I need > here is all lines after a line matching a given string, so that sed > command works, will fix it up to use that. Oops, indeed ;-) Thanks.
Re: [RFC/PATCH] builtin/blame: darken redundant line information
On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> But you do not want to (yet)? The goal is not to tell you where the bounds >> are, but the goal is to point out that extra care is required for review of >> these particular 3 lines. > > And when you _can_ help users in that "extra care" by pointing out > where the boundary is, what is the justification for hiding that > information? It is very complicated and confusing. Consider this: > context > -C > context > -B > -B > -B > -A > -A > -A > context > +A > +A > +A > +C > +B > +B > +B > context So from your emails I understood you want to markup block starts and ends, but in this case C is *both* start and end of a block, and has also different blocks around. So ideally we could tell the user this > context > _context C goes to after +A > -C > _context C goes to before +B > context > _context -B goes to after +C > -B > -B > -B > _context -B goes to before contextB > _context -A goes to after contextA > -A > -A > -A > _context -A goes to after contextA > context > _context +A comes from after -B > +A > +A > +A > _context +A comes from before contextA > _context +C comes from after contextC > +C > _context +C comes from before contextC > _context +B comes from after contextB > +B > +B > +B > _context +B comes from after -A > context (show the context of where the move is coming from/going to, maybe just one or two lines) And how many colors would be confusing for the user? I would think we want to start with a simple model and if a niche is not good (read: people think it can be improved easily compared to the usefulness they get out of it) enough we fix it up later. I thought that adding 4 new colors is already maybe too much, as Git users were happy with a handful for 10 years, so I am very opposed to add more than 4 colors unless there is a very good reason. And I'd think that this is not adding a lot of useful information for a reviewer? Thanks, Stefan
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes: > On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> But you do not want to (yet)? The goal is not to tell you where the bounds >>> are, but the goal is to point out that extra care is required for review of >>> these particular 3 lines. >> >> And when you _can_ help users in that "extra care" by pointing out >> where the boundary is, what is the justification for hiding that >> information? > > It is very complicated and confusing. Consider this: > >> context >> -C >> context >> -B >> -B >> -B >> -A >> -A >> -A >> context >> +A >> +A >> +A >> +C >> +B >> +B >> +B >> context > > So from your emails I understood you want to markup > block starts and ends, but in this case C is *both* start > and end of a block, and has also different blocks around. I never said "start and end" (you did). I just wanted the boundary of A and B and C clear, so I'd be perfectly happy with: context +A dim +A dim +A highlight #1 +C highlight #2 +B highlight #1 +B dim +B dim context You can do that still with only two highlight colors, no?
Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections
On Tue, Jun 13 2017, Junio C. Hamano jotted: > Ævar Arnfjörð Bjarmason writes: > >> On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano wrote: >>> Indenting using <<- would make it easier to read. I.e. >>> >>> cat >config.branch <<-\EOF && >>> ;; Comment for ... >>> [branch "source"] >>> ;; Comment for ... >>> ... >>> EOF >> >> I should have added a comment for that, I can't find any portable (but >> suggestions welcome) way to do that and preserve the indentation, so >> the test_cmp would still succeed if the moving/renaming function >> munged all leading whitespace in the config with -\EOF as opposed to >> \EOF. > > Ah, I see why it is done that way. You could indent the lines in > the configuration file with SPs (<<- strips only HTs, no?) Sure, we'll do that for v2. >> It's just a sufficiently large number, I thought -A was portable >> enough after grepping the test suite, but on closer inspection it >> turns out those were all git-grep invocations, oops. Yeah all I need >> here is all lines after a line matching a given string, so that sed >> command works, will fix it up to use that. > > Oops, indeed ;-) Thanks.
Re: [PATCH/RFC] branch: add tests for new copy branch feature
Hi, Ævar Arnfjörð Bjarmason wrote: > So the reason we have this for -m is: > > commit 3f59481e33 > Author: Jonathan Nieder > Date: Fri Nov 25 20:30:02 2011 -0600 > > branch: allow a no-op "branch -M HEAD" > > Overwriting the current branch with a different commit is forbidden, as it > will make the status recorded in the index and the working tree out of > sync with respect to the HEAD. There however is no reason to forbid it if > the current branch is renamed to itself, which admittedly is something > only an insane user would do, but is handy for scripts. > > My understanding of that last part is that Jonathan/someone (see > reported-by in that patch) had some script which was renaming > branches, and it was easier for whatever reason to just make it no-op > if the rename would have yielded the same result as doing nothing at > all. > > Most likely your implementation will consist of just re-using the > logic in rename_branch() (and renaming it to e.g. > copy_or_rename_branch() ...) so you could just re-use the no-op > behavior we use for -m, or if there's some reason not to no-op and > error instead for -c we could just do that, but in any case this case > of `git branch -c master master` or `git branch -c currentbranch` > should be tested for. I may be missing some context, but notice that the above mentioned commit is about -M, not -m. Thanks and hope that helps, Jonathan
Re: [RFC/PATCH] builtin/blame: darken redundant line information
On Tue, Jun 13, 2017 at 10:48 AM, Junio C Hamano wrote: > > I never said "start and end" (you did). I just wanted the boundary > of A and B and C clear, so I'd be perfectly happy with: > > context > +A dim > +A dim > +A highlight #1 > +C highlight #2 > +B highlight #1 > +B dim > +B dim > context > > You can do that still with only two highlight colors, no? So to put it into an algorithm: 1) detect blocks 2) if blocks are adjacent, their bounds are eligible for highlighting 3) the highlighting is implemented using the "alternate" strategy in that any line highlighted belonging to a different block flips the highlighting, such that: context +A dim +A dim +A highlight #1 +B highlight #2 +B dim +B dim context So if we go this way, we would need indeed 6 colors: Dimmed, Highlighted, HighlightedAlternative color-moved modes: nobounds:: uses dimmed only allbounds:: adjacentbounds:: See algorithm above, using dimmed for inside the block and both highlights for bounds, making sure adjacent block bounds alternate the highlighting color. alternate:: Uses only highlighting colors, complete block is colored with one of the highlights I think that is reasonable to implement. But I do still wonder if we really want to add so many new colors. I'll give it a try after my next submodule series. Thanks, Stefan
Re: [PATCH/RFC] branch: add tests for new copy branch feature
On Tue, Jun 13 2017, Jonathan Nieder jotted: > Hi, > > Ævar Arnfjörð Bjarmason wrote: > >> So the reason we have this for -m is: >> >> commit 3f59481e33 >> Author: Jonathan Nieder >> Date: Fri Nov 25 20:30:02 2011 -0600 >> >> branch: allow a no-op "branch -M HEAD" >> >> Overwriting the current branch with a different commit is forbidden, as >> it >> will make the status recorded in the index and the working tree out of >> sync with respect to the HEAD. There however is no reason to forbid it if >> the current branch is renamed to itself, which admittedly is something >> only an insane user would do, but is handy for scripts. >> >> My understanding of that last part is that Jonathan/someone (see >> reported-by in that patch) had some script which was renaming >> branches, and it was easier for whatever reason to just make it no-op >> if the rename would have yielded the same result as doing nothing at >> all. >> >> Most likely your implementation will consist of just re-using the >> logic in rename_branch() (and renaming it to e.g. >> copy_or_rename_branch() ...) so you could just re-use the no-op >> behavior we use for -m, or if there's some reason not to no-op and >> error instead for -c we could just do that, but in any case this case >> of `git branch -c master master` or `git branch -c currentbranch` >> should be tested for. > > I may be missing some context, but notice that the above mentioned > commit is about -M, not -m. The context was just that that commit added a change in how -M interacted when clobbering the current HEAD, and that -C should have a test for that behavior, which the patch now submitted to the list has: +test_expect_success 'git branch -C master master should work when master is checked out' ' + git checkout master && + git branch -C master master +'
Re: [RFC/PATCH] builtin/blame: darken redundant line information
Stefan Beller writes: > On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> But you do not want to (yet)? The goal is not to tell you where the bounds >>> are, but the goal is to point out that extra care is required for review of >>> these particular 3 lines. >> >> And when you _can_ help users in that "extra care" by pointing out >> where the boundary is, what is the justification for hiding that >> information? > ... > > And how many colors would be confusing for the user? Well, having two "dimmed" for moved lines are not needed if we use the same as context, so we can reduce the colors to just two highlights times two (i.e. old and new). Having said that, > I would think we want to start with a simple model ... would be OK. Let me configure the old-moved and new-moved to the same as context and see if I can live with just a single highlight color. Thanks.
Re: [PATCH/RFC] branch: add tests for new copy branch feature
Ævar Arnfjörð Bjarmason wrote: > On Tue, Jun 13 2017, Jonathan Nieder jotted: > > Ævar Arnfjörð Bjarmason wrote: >>> My understanding of that last part is that Jonathan/someone (see >>> reported-by in that patch) had some script which was renaming >>> branches, and it was easier for whatever reason to just make it no-op >>> if the rename would have yielded the same result as doing nothing at >>> all. >>> >>> Most likely your implementation will consist of just re-using the >>> logic in rename_branch() (and renaming it to e.g. >>> copy_or_rename_branch() ...) so you could just re-use the no-op >>> behavior we use for -m, or if there's some reason not to no-op and >>> error instead for -c we could just do that, but in any case this case >>> of `git branch -c master master` or `git branch -c currentbranch` >>> should be tested for. >> >> I may be missing some context, but notice that the above mentioned >> commit is about -M, not -m. > > The context was just that that commit added a change in how -M > interacted when clobbering the current HEAD, and that -C should have a > test for that behavior, which the patch now submitted to the list has: > > +test_expect_success 'git branch -C master master should work when master > is checked out' ' > + git checkout master && > + git branch -C master master > +' Perfect, thanks. Carry on. :) Jonathan
Re: [BUG] add_again() off-by-one error in custom format
Am 13.06.2017 um 00:49 schrieb Junio C Hamano:
Michael Giuffrida writes:
For the abbreviated commit hash placeholder ('h'), pretty.c uses
add_again() to cache the result of the expansion, and then uses that
result in future expansions. This causes problems when the expansion
includes whitespace:
$ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
newline:
a0b1c2d
no_newline:
a0b1c2
The second expansion of the hash added an unwanted newline and removed
the final character. It seemingly used the cached expansion *starting
from the inserted newline*.
The expected output is:
$ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
newline:
a0b1c2d
no_newline:a0b1c2d
Nicely explained. The add_again() mechanism caches an earlier
result by remembering the offset and the length in the strbuf the
formatted string is being accumulated to, but because %+x (and
probably %-x) magic placeholders futz with the result of
format_commit_one() in place, the cache goes out of sync, of course.
Indeed, a very nice bug report!
I think the call to format_commit_one() needs to be taught to pass
'magic' through, and then add_again() mechanism needs to be told not
to cache when magic is in effect, or something like that.
Perhaps something along this line...?
pretty.c | 64 ++--
1 file changed, 38 insertions(+), 26 deletions(-)
That looks quite big. You even invent a way to classify magic. Does the
caching feature justify the added complexity? Alternatives:
- Don't cache anymore, now that we have placeholders that change output
on top of the original append-only ones. Yields a net code reduction.
Duplicated %h, %t and %p will have to be resolved at each occurrence.
- Provide some space for the cache instead of reusing the output, like
a strbuf for each placeholder. Adds a memory allocation to each
first occurrence of %h, %t and %p. Such a cache could be reused for
multiple commits by truncating it instead of releasing; not sure how
to pass it in nicely for it to survive a sequence of calls, though.
René
Re: [PATCH v3 6/6] Use the early config machinery to expand aliases
On 06/13, Johannes Schindelin wrote:
> Instead of discovering the .git/ directory, read the config and then
> trying to painstakingly reset all the global state if we did not find a
> matching alias, let's use the early config machinery instead.
>
> It may look like unnecessary work to discover the .git/ directory in the
> early config machinery and then call setup_git_directory_gently() in the
> case of a shell alias, repeating the very same discovery *again*.
> However, we have to do this as the early config machinery takes pains
> *not* to touch any global state, while shell aliases expect a possibly
> changed working directory and at least the GIT_PREFIX and GIT_DIR
> variables to be set.
>
> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.
>
> This change also fixes a known issue where Git tried to read the pager
> config from an incorrect path in a subdirectory of a Git worktree if an
> alias expanded to a shell command.
>
> Signed-off-by: Johannes Schindelin
So because I've been looking at the config machinery lately, I've
noticed a lot of issues with how things are handled with respect to
gitdir vs commondir. Essentially the config resides at commondir/config
always, and only at gitdir/config when not working with a worktree.
Because of this, your patches point out a bug in how early config is
handled. I'll illustrate this using aliases.
Before this series (because aliases are read using the standard config
machinery):
> git init main
> git -C main config alias.test '!echo hello'
> git -C main test
hello
> git -C main worktree add ../worktree
> git -C worktree test
hello
After this series (using read_early_config()):
> git init main
> git -C main config alias.test '!echo hello'
> git -C main test
hello
> git -C main worktree add ../worktree
> git -C worktree test
git: 'test' is not a git command. See 'git --help'.
The issue is that read_early_config passes the gitdir and not the
commondir when reading the config.
The solution would be to add a 'commondir' field to the config_options
struct and populate that before reading the config. I'm planning on
fixing this in v2 of my config cleanup series which I'll hopefully have
finished by the end of the day.
> ---
> alias.c | 31 ---
> git.c| 55 ---
> t/t7006-pager.sh | 2 +-
> 3 files changed, 29 insertions(+), 59 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index 3b90397a99d..6bdc9363037 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,14 +1,31 @@
> #include "cache.h"
>
> +struct config_alias_data
> +{
> + struct strbuf key;
> + char *v;
> +};
> +
> +static int config_alias_cb(const char *key, const char *value, void *d)
> +{
> + struct config_alias_data *data = d;
> +
> + if (!strcmp(key, data->key.buf))
> + return git_config_string((const char **)&data->v, key, value);
> +
> + return 0;
> +}
> +
> char *alias_lookup(const char *alias)
> {
> - char *v = NULL;
> - struct strbuf key = STRBUF_INIT;
> - strbuf_addf(&key, "alias.%s", alias);
> - if (git_config_key_is_valid(key.buf))
> - git_config_get_string(key.buf, &v);
> - strbuf_release(&key);
> - return v;
> + struct config_alias_data data = { STRBUF_INIT, NULL };
> +
> + strbuf_addf(&data.key, "alias.%s", alias);
> + if (git_config_key_is_valid(data.key.buf))
> + read_early_config(config_alias_cb, &data);
> + strbuf_release(&data.key);
> +
> + return data.v;
> }
>
> #define SPLIT_CMDLINE_BAD_ENDING 1
> diff --git a/git.c b/git.c
> index 8ff44f081d4..58ef570294d 100644
> --- a/git.c
> +++ b/git.c
> @@ -16,50 +16,6 @@ const char git_more_info_string[] =
> "to read about a specific subcommand or concept.");
>
> static int use_pager = -1;
> -static char *orig_cwd;
> -static const char *env_names[] = {
> - GIT_DIR_ENVIRONMENT,
> - GIT_WORK_TREE_ENVIRONMENT,
> - GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
> - GIT_PREFIX_ENVIRONMENT
> -};
> -static char *orig_env[4];
> -static int save_restore_env_balance;
> -
> -static void save_env_before_alias(void)
> -{
> - int i;
> -
> - assert(save_restore_env_balance == 0);
> - save_restore_env_balance = 1;
> - orig_cwd = xgetcwd();
> - for (i = 0; i < ARRAY_SIZE(env_names); i++) {
> - orig_env[i] = getenv(env_names[i]);
> - orig_env[i] = xstrdup_or_null(orig_env[i]);
> - }
> -}
> -
> -static void restore_env(int external_alias)
> -{
> - int i;
> -
> - assert(save_restore_env_balance == 1);
> - save_restore_env_balance = 0;
> - if (!external_alias && orig_cwd && chdir(orig_cwd))
> - die_errno("cou
Re: [BUG] add_again() off-by-one error in custom format
René Scharfe writes: > Indeed, a very nice bug report! > >> I think the call to format_commit_one() needs to be taught to pass >> 'magic' through, and then add_again() mechanism needs to be told not >> to cache when magic is in effect, or something like that. >> >> Perhaps something along this line...? >> >> pretty.c | 64 >> ++-- >> 1 file changed, 38 insertions(+), 26 deletions(-) > > That looks quite big. You even invent a way to classify magic. Hmph, by "a way to classify", do you mean the enum? That thing was there from before, just moved. Also I think we do not have to change add_again() at all, because chunk->off tolerates being given a garbage value as long as chunk->len stays 0, and add_again() does not change chunk->len at all. Which cuts the diffstat down to pretty.c | 40 +--- 1 file changed, 25 insertions(+), 15 deletions(-) > Does the caching feature justify the added complexity? That's a good question. I thought about your second alternative while adding the "don't cache" thing, but if we can measure and find out that we are not gaining much from caching, certainly that sounds much simpler. > > - Don't cache anymore, now that we have placeholders that change output > on top of the original append-only ones. Yields a net code reduction. > Duplicated %h, %t and %p will have to be resolved at each occurrence. > > - Provide some space for the cache instead of reusing the output, like > a strbuf for each placeholder. Adds a memory allocation to each > first occurrence of %h, %t and %p. Such a cache could be reused for > multiple commits by truncating it instead of releasing; not sure how > to pass it in nicely for it to survive a sequence of calls, though. > > René
Re: [RFC/PATCH] submodules: overhaul documentation
Stefan Beller writes: > @@ -149,15 +119,17 @@ deinit [-f|--force] (--all|[--] ...):: > tree. Further calls to `git submodule update`, `git submodule foreach` > and `git submodule sync` will skip any unregistered submodules until > they are initialized again, so use this command if you don't want to > - have a local checkout of the submodule in your working tree anymore. If > - you really want to remove a submodule from the repository and commit > - that use linkgit:git-rm[1] instead. > + have a local checkout of the submodule in your working tree anymore. > + > When the command is run without pathspec, it errors out, > instead of deinit-ing everything, to prevent mistakes. > + > If `--force` is specified, the submodule's working tree will > be removed even if it contains local modifications. > ++ > +If you really want to remove a submodule from the repository and commit > +that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal > +options. Good reorganization. > diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt > new file mode 100644 > index 00..2bf3149b68 > --- /dev/null > +++ b/Documentation/gitsubmodules.txt > @@ -0,0 +1,214 @@ > +gitsubmodules(7) > + > + > +NAME > + > +gitsubmodules - mounting one repository inside another > + > +SYNOPSIS > + > +.gitmodules, $GIT_DIR/config > +-- > +git submodule > +git --recurse-submodules > +-- > + > +DESCRIPTION > +--- > + > +A submodule is another Git repository tracked in a subdirectory of your > +repository. The tracked repository has its own history, which does not > +interfere with the history of the current repository. "tracked in a subdirectory" sounds as if your top-level superproject has a dedicated submodules/ directory and in it there live a bunch of submodules. Which obviously is not what you meant. If phrased "tracked as a subdirectory", I think the sentence makes sense. While "which does not interfere" may be technically correct, I am not sure what the value of saying that is. > +Submodules are composed from a so-called `gitlink` tree entry > +in the main repository that refers to a particular commit object > +within the inner repository. Correct, but it may be unclear to the readers why we do so. Perhaps ... and this way, the tree of each commit in the main repository "knows" which commit from the submodule's history is "tied" to it. or something like that? > +Additionally to the gitlink entry the `.gitmodules` file (see > +linkgit:gitmodules[5]) at the root of the source tree contains > +information needed for submodules. Is that really true? Each submodule do not *need* what is in .gitmodules; the top-level superproject needs to learn about its submodules from the contents of that file, though. > +The only required information > +is the path setting, which estabishes a logical name for the submodule. The phrase "the path setting" feels a bit unfortunate. Is that "only" thing we need? Without URL we have no way to populate it, no? > +The usual git configuration (see linkgit:git-config[1]) can be used to > +override settings given by the `.gitmodules` file. > + > +Submodules can be used for two different use cases: > + > +1. Using another project that stands on its own. > + When you want to use a third party library, submodules allow you to > + have a clean history for your own project as well as for the library. > + This also allows for updating the third party library as needed. > + > +2. Artificially split a (logically single) project into multiple > + repositories and tying them back together. This can be used to > + overcome deficiences in the data model of Git, such as: s/deficiences in the data model/current limitations/ perhaps? > +* To have finer grained access control. > + The design principles of Git do not allow for partial repositories to be > + checked out or transferred. A repository is the smallest unit that a user > + can be given access to. Submodules are separate repositories, such that > + you can restrict access to parts of your project via the use of submodules. Some servers implement per-branch access control that seems to work rather well. Given that "shallow history" is possible (i.e. you could give one commit without exposing older parts of the history), I think the limitation this paragrah refers to is that "a tree is the smallest unit that the user can be given access to." > +* In its current form Git scales up poorly for very large repositories that > + change a lot, as the history grows very large. For that you may want to > look > + at shallow clone, sparse checkout, or git-LFS. > + However you can also use submodules to e.g. hold large binary assets > + and these repositories are then shallowly cloned such that you do not > + have a large history locally. This is why I suggest "current limitations
Re: [PATCH v4 1/5] stash: add test for stash create with no files
Joel Teichroeb writes: > Ensure the command gives the correct return code OK. When you know what the correct return code is, we'd prefer to see it spelled out, i.e. Ensure that the command succeeds. Or did you mean that the command outputs nothing? The test itself looks obviously correct ;-) > Signed-off-by: Joel Teichroeb > --- > t/t3903-stash.sh | 8 > 1 file changed, 8 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 3b4bed5c9a..cc923e6335 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' ' > test foo = "$(cat file/file)" > ' > > +test_expect_success 'stash create - no changes' ' > + git stash clear && > + test_when_finished "git reset --hard HEAD" && > + git reset --hard && > + git stash create >actual && > + test_must_be_empty actual > +' > + > test_expect_success 'stash branch - no stashes on stack, stash-like > argument' ' > git stash clear && > test_when_finished "git reset --hard HEAD" &&
Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch
Joel Teichroeb writes:
> If the return value of merge recurisve is not checked, the stash could end
> up being dropped even though it was not applied properly
s/recurisve/recursive/
> Signed-off-by: Joel Teichroeb
> ---
> t/t3903-stash.sh | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index cc923e6335..5399fb05ca 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the
> stash if the branch exists
> git rev-parse stash@{0} --
> '
>
> +test_expect_success 'stash branch should not drop the stash if the apply
> fails' '
> + git stash clear &&
> + git reset HEAD~1 --hard &&
> + echo foo >file &&
> + git add file &&
> + git commit -m initial &&
It's not quite intuitive to call a non-root commit "initial" ;-)
> + echo bar >file &&
> + git stash &&
> + echo baz >file &&
OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.
> + test_when_finished "git checkout master" &&
> + test_must_fail git stash branch new_branch stash@{0} &&
Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and
unstash, but because 'file' in the working tree is dirty, we fail to
apply the stash and stop?
This sounds like a bug to me. Shouldn't we be staying on 'master',
and fail without even creating 'new_branch', when this happens?
In any case we should be testing what branch we are on after this
step. What branch should we be on after "git stash branch" fails?
> + git rev-parse stash@{0} --
> +'
> +
> test_expect_success 'stash apply shows status same as git status (relative
> to current directory)' '
> git stash clear &&
> echo 1 >subdir/subfile1 &&
Re: [PATCH v4 3/5] stash: add test for stashing in a detached state
Joel Teichroeb writes:
> Signed-off-by: Joel Teichroeb
> ---
> t/t3903-stash.sh | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 5399fb05ca..ce4c8fe3d6 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for
> the message' '
> test_cmp expect actual
> '
>
> +test_expect_success 'create in a detached state' '
> + test_when_finished "git checkout master" &&
> + git checkout HEAD~1 &&
> + >foo &&
> + git add foo &&
> + STASH_ID=$(git stash create) &&
> + HEAD_ID=$(git rev-parse --short HEAD) &&
> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
> + git show --pretty=%s -s ${STASH_ID} >actual &&
> + test_cmp expect actual
> +'
Hmph. Is the title automatically given to the stash the
only/primary thing that is of interest to us in this test? I think
we care more about that we record the right thing in the resulting
stash and also after creating the stash the working tree and the
index becomes clean. Shouldn't we be testing that?
If "git stash create" fails to make the working tree and the index
clean, then "git checkout master" run by when-finished will carry
the local modifications with us, which probably is not what you
meant. You'd need "reset --hard" there, too, perhaps?
> test_expect_success 'stash -- stashes and restores the file' '
> >foo &&
> >bar &&
Re: [PATCH v4 4/5] merge: close the index lock when not writing the new index
Joel Teichroeb writes:
> If the merge does not have anything to do, it does not unlock the index,
> causing any further index operations to fail. Thus, always unlock the index
> regardless of outcome.
>
> Signed-off-by: Joel Teichroeb
> ---
This one makes sense.
So far, nobody who calls this function performs further index
manipulations and letting the atexit handlers automatically release
the lock was sufficient. This allows new callers to do more work on
the index after a merge finishes.
> merge-recursive.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ae5238d82c..16bb5512ef 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o,
> if (clean < 0)
> return clean;
>
> - if (active_cache_changed &&
> - write_locked_index(&the_index, lock, COMMIT_LOCK))
> - return err(o, _("Unable to write index."));
> + if (active_cache_changed) {
> + if (write_locked_index(&the_index, lock, COMMIT_LOCK))
> + return err(o, _("Unable to write index."));
> + } else {
> + rollback_lock_file(lock);
> + }
>
> return clean ? 0 : 1;
> }
Re: [PATCH v4 3/5] stash: add test for stashing in a detached state
On Tue, Jun 13, 2017 at 12:45 PM, Junio C Hamano wrote:
> Joel Teichroeb writes:
>
>> Signed-off-by: Joel Teichroeb
>> ---
>> t/t3903-stash.sh | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index 5399fb05ca..ce4c8fe3d6 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for
>> the message' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'create in a detached state' '
>> + test_when_finished "git checkout master" &&
>> + git checkout HEAD~1 &&
>> + >foo &&
>> + git add foo &&
>> + STASH_ID=$(git stash create) &&
>> + HEAD_ID=$(git rev-parse --short HEAD) &&
>> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>> + git show --pretty=%s -s ${STASH_ID} >actual &&
>> + test_cmp expect actual
>> +'
>
> Hmph. Is the title automatically given to the stash the
> only/primary thing that is of interest to us in this test? I think
> we care more about that we record the right thing in the resulting
> stash and also after creating the stash the working tree and the
> index becomes clean. Shouldn't we be testing that?
In this case, the title is really what I wanted to test. There are
other tests already to make sure that stash create works, but there
were no tests to ensure that a stash was created with the correct
title when not on a branch. That being said though, I'll add more
validation as more validation is always better.
>
> If "git stash create" fails to make the working tree and the index
> clean, then "git checkout master" run by when-finished will carry
> the local modifications with us, which probably is not what you
> meant. You'd need "reset --hard" there, too, perhaps?
Agreed.
>
>> test_expect_success 'stash -- stashes and restores the file' '
>> >foo &&
>> >bar &&
Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch
On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamano wrote:
> Joel Teichroeb writes:
>
>> If the return value of merge recurisve is not checked, the stash could end
>> up being dropped even though it was not applied properly
>
> s/recurisve/recursive/
>
>> Signed-off-by: Joel Teichroeb
>> ---
>> t/t3903-stash.sh | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index cc923e6335..5399fb05ca 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the
>> stash if the branch exists
>> git rev-parse stash@{0} --
>> '
>>
>> +test_expect_success 'stash branch should not drop the stash if the apply
>> fails' '
>> + git stash clear &&
>> + git reset HEAD~1 --hard &&
>> + echo foo >file &&
>> + git add file &&
>> + git commit -m initial &&
>
> It's not quite intuitive to call a non-root commit "initial" ;-)
>
>> + echo bar >file &&
>> + git stash &&
>> + echo baz >file &&
>
> OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.
>
>> + test_when_finished "git checkout master" &&
>> + test_must_fail git stash branch new_branch stash@{0} &&
>
> Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and
> unstash, but because 'file' in the working tree is dirty, we fail to
> apply the stash and stop?
>
> This sounds like a bug to me. Shouldn't we be staying on 'master',
> and fail without even creating 'new_branch', when this happens?
Good point. The existing behavior is to create new_branch and check it
out. I'm not sure what the correct state should be then. Create
new_branch, checkout new_branch, fail to apply, checkout master?
Should it then delete new_branch? Is there a way instead to test
applying the stash before creating the branch without actually
applying it? Something like putting merge_recursive into some kind of
dry-run mode?
>
> In any case we should be testing what branch we are on after this
> step. What branch should we be on after "git stash branch" fails?
>
>> + git rev-parse stash@{0} --
>> +'
>> +
>> test_expect_success 'stash apply shows status same as git status (relative
>> to current directory)' '
>> git stash clear &&
>> echo 1 >subdir/subfile1 &&
Re: [BUG] add_again() off-by-one error in custom format
Am 13.06.2017 um 20:29 schrieb Junio C Hamano: René Scharfe writes: Indeed, a very nice bug report! I think the call to format_commit_one() needs to be taught to pass 'magic' through, and then add_again() mechanism needs to be told not to cache when magic is in effect, or something like that. Perhaps something along this line...? pretty.c | 64 ++-- 1 file changed, 38 insertions(+), 26 deletions(-) That looks quite big. You even invent a way to classify magic. Hmph, by "a way to classify", do you mean the enum? That thing was there from before, just moved. Oh, yes, sorry. I didn't even get that far into the patch. (I'll better go to bed after hitting send..) Also I think we do not have to change add_again() at all, because chunk->off tolerates being given a garbage value as long as chunk->len stays 0, and add_again() does not change chunk->len at all. Which cuts the diffstat down to pretty.c | 40 +--- 1 file changed, 25 insertions(+), 15 deletions(-) Does the caching feature justify the added complexity? That's a good question. I thought about your second alternative while adding the "don't cache" thing, but if we can measure and find out that we are not gaining much from caching, certainly that sounds much simpler. The difference is about the same as the one between: $ time git log --format="" >/dev/null real0m0.463s user0m0.448s sys 0m0.012s and: $ time git log --format="%h" >/dev/null real0m1.062s user0m0.636s sys 0m0.416s With caching duplicates are basically free and without it short hashes have to be looked up again. Other placeholders may reduce the relative slowdown, depending on how expensive they are. Forgot a third option, probably because it's not a particularly good idea: Replacing the caching in pretty.c with a short static cache in find_unique_abbrev_r(). René
Re: [PATCH v4 3/5] stash: add test for stashing in a detached state
Joel Teichroeb writes:
>>> +test_expect_success 'create in a detached state' '
>>> + test_when_finished "git checkout master" &&
>>> + git checkout HEAD~1 &&
>>> + >foo &&
>>> + git add foo &&
>>> + STASH_ID=$(git stash create) &&
>>> + HEAD_ID=$(git rev-parse --short HEAD) &&
>>> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>>> + git show --pretty=%s -s ${STASH_ID} >actual &&
>>> + test_cmp expect actual
>>> +'
>>
>> Hmph. Is the title automatically given to the stash the
>> only/primary thing that is of interest to us in this test? I think
>> we care more about that we record the right thing in the resulting
>> stash and also after creating the stash the working tree and the
>> index becomes clean. Shouldn't we be testing that?
>
> In this case, the title is really what I wanted to test. There are
> other tests already to make sure that stash create works, but there
> were no tests to ensure that a stash was created with the correct
> title when not on a branch.
Ah, OK.
Thanks.
[PATCH v2 5/6] setup: teach discover_git_directory to respect the commondir
Currently 'discover_git_directory' only looks at the gitdir to determine
if a git directory was discovered. This causes a problem in the event
that the gitdir which was discovered was in fact a per-worktree git
directory and not the common git directory. This is because the
repository config, which is checked to verify the repository's format,
is stored in the commondir and not in the per-worktree gitdir. Correct
this behavior by checking the config stored in the commondir.
It will also be of use for callers to have access to the commondir, so
lets also return that upon successfully discovering a git directory.
Signed-off-by: Brandon Williams
---
cache.h | 3 ++-
config.c | 10 ++
setup.c | 9 +++--
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index fd45b8c55..a4176436d 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,8 @@ extern void setup_work_tree(void);
* appended to gitdir. The return value is either NULL if no repository was
* found, or pointing to the path inside gitdir's buffer.
*/
-extern const char *discover_git_directory(struct strbuf *gitdir);
+extern const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/config.c b/config.c
index 4e2842689..9aa9b9715 100644
--- a/config.c
+++ b/config.c
@@ -1652,7 +1652,8 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
void read_early_config(config_fn_t cb, void *data)
{
struct config_options opts = {0};
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf commondir = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
opts.respect_includes = 1;
@@ -1666,12 +1667,13 @@ void read_early_config(config_fn_t cb, void *data)
* notably, the current working directory is still the same after the
* call).
*/
- else if (discover_git_directory(&buf))
- opts.git_dir = buf.buf;
+ else if (discover_git_directory(&commondir, &gitdir))
+ opts.git_dir = gitdir.buf;
git_config_with_options(cb, data, NULL, &opts);
- strbuf_release(&buf);
+ strbuf_release(&commondir);
+ strbuf_release(&gitdir);
}
static void git_config_check_init(void);
diff --git a/setup.c b/setup.c
index e99a82cbe..7bbb8736f 100644
--- a/setup.c
+++ b/setup.c
@@ -946,10 +946,12 @@ static enum discovery_result
setup_git_directory_gently_1(struct strbuf *dir,
}
}
-const char *discover_git_directory(struct strbuf *gitdir)
+const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir)
{
struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+ size_t commondir_offset = commondir->len;
struct repository_format candidate;
if (strbuf_getcwd(&dir))
@@ -974,8 +976,10 @@ const char *discover_git_directory(struct strbuf *gitdir)
strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
}
+ get_common_dir(commondir, gitdir->buf + gitdir_offset);
+
strbuf_reset(&dir);
- strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
+ strbuf_addf(&dir, "%s/config", commondir->buf + commondir_offset);
read_repository_format(&candidate, dir.buf);
strbuf_release(&dir);
@@ -983,6 +987,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
warning("ignoring git dir '%s': %s",
gitdir->buf + gitdir_offset, err.buf);
strbuf_release(&err);
+ strbuf_setlen(commondir, commondir_offset);
return NULL;
}
--
2.13.1.518.g3df882009-goog
[PATCH v2 0/6] config.h
Changes in v2:
* Fix a small nit in builtin/config.c that Jonathan pointed out.
* Added two patches which ensure that the repository wide config is properly
read by providing 'commondir' as a field in the 'config_options' struct.
Brandon Williams (6):
config: create config.h
config: remove git_config_iter
config: don't include config.h by default
config: don't implicitly use gitdir
setup: teach discover_git_directory to respect the commondir
config: respect commondir
--- interdiff with 'origin/bw/config-h'
diff --git a/builtin/config.c b/builtin/config.c
index 90f49a6ee..8b6e227c5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,8 +539,10 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
- if (have_git_dir())
- config_options.git_dir = get_git_common_dir();
+ if (!nongit) {
+ config_options.commondir = get_git_common_dir();
+ config_options.git_dir = get_git_dir();
+ }
if (end_null) {
term = '\0';
diff --git a/cache.h b/cache.h
index fd45b8c55..a4176436d 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,8 @@ extern void setup_work_tree(void);
* appended to gitdir. The return value is either NULL if no repository was
* found, or pointing to the path inside gitdir's buffer.
*/
-extern const char *discover_git_directory(struct strbuf *gitdir);
+extern const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/config.c b/config.c
index 4e2842689..81151fb54 100644
--- a/config.c
+++ b/config.c
@@ -1544,8 +1544,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
- if (opts->git_dir)
- repo_config = mkpathdup("%s/config", opts->git_dir);
+ if (opts->commondir)
+ repo_config = mkpathdup("%s/config", opts->commondir);
else
repo_config = NULL;
@@ -1609,8 +1609,11 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
opts.respect_includes = 1;
- if (have_git_dir())
- opts.git_dir = get_git_common_dir();
+ if (have_git_dir()) {
+ opts.commondir = get_git_common_dir();
+ opts.git_dir = get_git_dir();
+ }
+
if (git_config_with_options(fn, data, NULL, &opts) < 0)
/*
* git_config_with_options() normally returns only
@@ -1652,11 +1655,13 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
void read_early_config(config_fn_t cb, void *data)
{
struct config_options opts = {0};
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf commondir = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
opts.respect_includes = 1;
- if (have_git_dir())
+ if (have_git_dir()) {
+ opts.commondir = get_git_common_dir();
opts.git_dir = get_git_dir();
/*
* When setup_git_directory() was not yet asked to discover the
@@ -1666,12 +1671,15 @@ void read_early_config(config_fn_t cb, void *data)
* notably, the current working directory is still the same after the
* call).
*/
- else if (discover_git_directory(&buf))
- opts.git_dir = buf.buf;
+ } else if (discover_git_directory(&commondir, &gitdir)) {
+ opts.commondir = commondir.buf;
+ opts.git_dir = gitdir.buf;
+ }
git_config_with_options(cb, data, NULL, &opts);
- strbuf_release(&buf);
+ strbuf_release(&commondir);
+ strbuf_release(&gitdir);
}
static void git_config_check_init(void);
diff --git a/config.h b/config.h
index c70599bd5..63b92784c 100644
--- a/config.h
+++ b/config.h
@@ -30,6 +30,7 @@ enum config_origin_type {
struct config_options {
unsigned int respect_includes : 1;
+ const char *commondir;
const char *git_dir;
};
diff --git a/setup.c b/setup.c
index e99a82cbe..7bbb8736f 100644
--- a/setup.c
+++ b/setup.c
@@ -946,10 +946,12 @@ static enum discovery_result
setup_git_directory_gently_1(struct strbuf *dir,
}
}
-const char *discover_git_directory(struct strbuf *gitdir)
+const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir)
{
struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+ size_t commondir_
[PATCH v2 1/6] config: create config.h
Move all config related declarations from cache.h to a new config.h
header file. This makes cache.h smaller and allows for the opportunity
in a following patch to only include config.h when needed.
Signed-off-by: Brandon Williams
---
cache.h | 190 +
config.h | 194 +++
2 files changed, 195 insertions(+), 189 deletions(-)
create mode 100644 config.h
diff --git a/cache.h b/cache.h
index 4d92aae0e..ed56f8818 100644
--- a/cache.h
+++ b/cache.h
@@ -11,6 +11,7 @@
#include "string-list.h"
#include "pack-revindex.h"
#include "hash.h"
+#include "config.h"
#ifndef platform_SHA_CTX
/*
@@ -1872,188 +1873,9 @@ extern int packed_object_info(struct packed_git *pack,
off_t offset, struct obje
/* Dumb servers support */
extern int update_server_info(int);
-/* git_config_parse_key() returns these negated: */
-#define CONFIG_INVALID_KEY 1
-#define CONFIG_NO_SECTION_OR_NAME 2
-/* git_config_set_gently(), git_config_set_multivar_gently() return the above
or these: */
-#define CONFIG_NO_LOCK -1
-#define CONFIG_INVALID_FILE 3
-#define CONFIG_NO_WRITE 4
-#define CONFIG_NOTHING_SET 5
-#define CONFIG_INVALID_PATTERN 6
-#define CONFIG_GENERIC_ERROR 7
-
-#define CONFIG_REGEX_NONE ((void *)1)
-
-struct git_config_source {
- unsigned int use_stdin:1;
- const char *file;
- const char *blob;
-};
-
-enum config_origin_type {
- CONFIG_ORIGIN_BLOB,
- CONFIG_ORIGIN_FILE,
- CONFIG_ORIGIN_STDIN,
- CONFIG_ORIGIN_SUBMODULE_BLOB,
- CONFIG_ORIGIN_CMDLINE
-};
-
-struct config_options {
- unsigned int respect_includes : 1;
- const char *git_dir;
-};
-
-typedef int (*config_fn_t)(const char *, const char *, void *);
-extern int git_default_config(const char *, const char *, void *);
-extern int git_config_from_file(config_fn_t fn, const char *, void *);
-extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
- const char *name, const char *buf,
size_t len, void *data);
-extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
-const unsigned char *sha1, void *data);
-extern void git_config_push_parameter(const char *text);
-extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern void read_early_config(config_fn_t cb, void *data);
-extern void git_config(config_fn_t fn, void *);
-extern int git_config_with_options(config_fn_t fn, void *,
- struct git_config_source *config_source,
- const struct config_options *opts);
-extern int git_parse_ulong(const char *, unsigned long *);
-extern int git_parse_maybe_bool(const char *);
-extern int git_config_int(const char *, const char *);
-extern int64_t git_config_int64(const char *, const char *);
-extern unsigned long git_config_ulong(const char *, const char *);
-extern ssize_t git_config_ssize_t(const char *, const char *);
-extern int git_config_bool_or_int(const char *, const char *, int *);
-extern int git_config_bool(const char *, const char *);
-extern int git_config_maybe_bool(const char *, const char *);
-extern int git_config_string(const char **, const char *, const char *);
-extern int git_config_pathname(const char **, const char *, const char *);
-extern int git_config_set_in_file_gently(const char *, const char *, const
char *);
-extern void git_config_set_in_file(const char *, const char *, const char *);
-extern int git_config_set_gently(const char *, const char *);
-extern void git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
-extern int git_config_key_is_valid(const char *key);
-extern int git_config_set_multivar_gently(const char *, const char *, const
char *, int);
-extern void git_config_set_multivar(const char *, const char *, const char *,
int);
-extern int git_config_set_multivar_in_file_gently(const char *, const char *,
const char *, const char *, int);
-extern void git_config_set_multivar_in_file(const char *, const char *, const
char *, const char *, int);
-extern int git_config_rename_section(const char *, const char *);
-extern int git_config_rename_section_in_file(const char *, const char *, const
char *);
-extern const char *git_etc_gitconfig(void);
-extern int git_env_bool(const char *, int);
-extern unsigned long git_env_ulong(const char *, unsigned long);
-extern int git_config_system(void);
-extern int config_error_nonbool(const char *);
-#if defined(__GNUC__)
-#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
-#endif
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
-extern int git_config_parse_parameter(const char *, config_fn_t fn, void
*data);
-
-enum config_scope {
- CONFIG_SCOPE_UNKNOWN = 0,
- CONFIG_SCOPE_SYSTEM,
-
[PATCH v2 2/6] config: remove git_config_iter
Since there is no implementation of the function 'git_config_iter' lets stop exporting it and remove the prototype from config.h. Signed-off-by: Brandon Williams --- config.h | 1 - 1 file changed, 1 deletion(-) diff --git a/config.h b/config.h index f7f8b66c5..c70599bd5 100644 --- a/config.h +++ b/config.h @@ -165,7 +165,6 @@ extern int git_configset_get_pathname(struct config_set *cs, const char *key, co extern int git_config_get_value(const char *key, const char **value); extern const struct string_list *git_config_get_value_multi(const char *key); extern void git_config_clear(void); -extern void git_config_iter(config_fn_t fn, void *data); extern int git_config_get_string_const(const char *key, const char **dest); extern int git_config_get_string(const char *key, char **dest); extern int git_config_get_int(const char *key, int *dest); -- 2.13.1.518.g3df882009-goog
[PATCH v2 6/6] config: respect commondir
Worktrees present an interesting problem when it comes to the config.
Historically we could assume that the per-repository config lives at
'gitdir/config', but since worktrees were introduced this isn't the case
anymore. There is currently no way to specify per-worktree
configuration, and as such the repository config is shared with all
worktrees and is located at 'commondir/config'.
Many users of the config machinery correctly set
'config_options.git_dir' with the repository's commondir, allowing the
config to be properly loaded when operating in a worktree. But other's,
like 'read_early_config()', set 'config_options.git_dir' with the
repository's gitdir which can be incorrect when using worktrees.
To fix this issue, and to make things less ambiguous, lets add a
'commondir' field to the 'config_options' struct and have all callers
properly set both the 'git_dir' and 'commondir' fields so that the
config machinery is able to properly find the repository's config.
Signed-off-by: Brandon Williams
---
builtin/config.c | 6 --
config.c | 18 --
config.h | 1 +
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index f06a8dff2..8b6e227c5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,8 +539,10 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
- if (!nongit)
- config_options.git_dir = get_git_common_dir();
+ if (!nongit) {
+ config_options.commondir = get_git_common_dir();
+ config_options.git_dir = get_git_dir();
+ }
if (end_null) {
term = '\0';
diff --git a/config.c b/config.c
index 9aa9b9715..81151fb54 100644
--- a/config.c
+++ b/config.c
@@ -1544,8 +1544,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
- if (opts->git_dir)
- repo_config = mkpathdup("%s/config", opts->git_dir);
+ if (opts->commondir)
+ repo_config = mkpathdup("%s/config", opts->commondir);
else
repo_config = NULL;
@@ -1609,8 +1609,11 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
opts.respect_includes = 1;
- if (have_git_dir())
- opts.git_dir = get_git_common_dir();
+ if (have_git_dir()) {
+ opts.commondir = get_git_common_dir();
+ opts.git_dir = get_git_dir();
+ }
+
if (git_config_with_options(fn, data, NULL, &opts) < 0)
/*
* git_config_with_options() normally returns only
@@ -1657,7 +1660,8 @@ void read_early_config(config_fn_t cb, void *data)
opts.respect_includes = 1;
- if (have_git_dir())
+ if (have_git_dir()) {
+ opts.commondir = get_git_common_dir();
opts.git_dir = get_git_dir();
/*
* When setup_git_directory() was not yet asked to discover the
@@ -1667,8 +1671,10 @@ void read_early_config(config_fn_t cb, void *data)
* notably, the current working directory is still the same after the
* call).
*/
- else if (discover_git_directory(&commondir, &gitdir))
+ } else if (discover_git_directory(&commondir, &gitdir)) {
+ opts.commondir = commondir.buf;
opts.git_dir = gitdir.buf;
+ }
git_config_with_options(cb, data, NULL, &opts);
diff --git a/config.h b/config.h
index c70599bd5..63b92784c 100644
--- a/config.h
+++ b/config.h
@@ -30,6 +30,7 @@ enum config_origin_type {
struct config_options {
unsigned int respect_includes : 1;
+ const char *commondir;
const char *git_dir;
};
--
2.13.1.518.g3df882009-goog
[PATCH v2 3/6] config: don't include config.h by default
Stop including config.h by default in cache.h. Instead only include config.h in those files which require use of the config system. Signed-off-by: Brandon Williams --- advice.c | 1 + alias.c | 1 + apply.c | 1 + archive-tar.c| 1 + archive-zip.c| 1 + archive.c| 1 + attr.c | 1 + bisect.c | 1 + branch.c | 1 + builtin/add.c| 1 + builtin/am.c | 1 + builtin/blame.c | 2 ++ builtin/branch.c | 1 + builtin/cat-file.c | 1 + builtin/check-attr.c | 1 + builtin/check-ignore.c | 1 + builtin/check-mailmap.c | 1 + builtin/checkout-index.c | 1 + builtin/checkout.c | 1 + builtin/clean.c | 1 + builtin/clone.c | 1 + builtin/column.c | 1 + builtin/commit-tree.c| 1 + builtin/commit.c | 1 + builtin/config.c | 1 + builtin/count-objects.c | 1 + builtin/describe.c | 1 + builtin/diff-files.c | 1 + builtin/diff-index.c | 1 + builtin/diff-tree.c | 1 + builtin/diff.c | 1 + builtin/difftool.c | 1 + builtin/fast-export.c| 1 + builtin/fetch.c | 1 + builtin/fmt-merge-msg.c | 1 + builtin/for-each-ref.c | 1 + builtin/fsck.c | 1 + builtin/gc.c | 1 + builtin/grep.c | 1 + builtin/hash-object.c| 1 + builtin/help.c | 1 + builtin/index-pack.c | 1 + builtin/init-db.c| 1 + builtin/log.c| 1 + builtin/ls-files.c | 1 + builtin/ls-tree.c| 1 + builtin/merge-base.c | 1 + builtin/merge-file.c | 1 + builtin/merge.c | 1 + builtin/mv.c | 1 + builtin/name-rev.c | 1 + builtin/notes.c | 1 + builtin/pack-objects.c | 1 + builtin/patch-id.c | 1 + builtin/pull.c | 1 + builtin/push.c | 1 + builtin/read-tree.c | 1 + builtin/rebase--helper.c | 1 + builtin/receive-pack.c | 1 + builtin/reflog.c | 1 + builtin/remote.c | 1 + builtin/repack.c | 1 + builtin/replace.c| 1 + builtin/rerere.c | 1 + builtin/reset.c | 1 + builtin/rev-list.c | 1 + builtin/rev-parse.c | 1 + builtin/revert.c | 1 + builtin/rm.c | 1 + builtin/send-pack.c | 1 + builtin/shortlog.c | 1 + builtin/show-branch.c| 1 + builtin/stripspace.c | 1 + builtin/submodule--helper.c | 1 + builtin/symbolic-ref.c | 1 + builtin/tag.c| 1 + builtin/unpack-file.c| 1 + builtin/unpack-objects.c | 1 + builtin/update-index.c | 1 + builtin/update-ref.c | 1 + builtin/update-server-info.c | 1 + builtin/var.c| 1 + builtin/verify-commit.c | 1 + builtin/verify-pack.c| 1 + builtin/verify-tag.c | 1 + builtin/worktree.c | 1 + builtin/write-tree.c | 1 + cache.h | 1 - color.c | 1 + column.c | 1 + config.c | 1 + connect.c| 1 + convert.c| 1 + credential-cache--daemon.c | 1 + credential.c | 1 + daemon.c | 1 + diff.c | 1 + dir.c| 1 + environment.c| 1 + fast-import.c| 1 + fetch-pack.c | 1 + git.c| 1 + gpg-interface.c | 1 + graph.c | 1 + grep.c | 1 + help.c | 1 + http-backend.c | 1 + http-fetch.c | 1 + http.c | 1 + ident.c | 1 + imap-send.c | 1 + ll-merge.c | 1 + log-tree.c | 1 + mailinfo.c | 1 + merge-recursive.c| 1 + notes-utils.c| 1 + notes.c | 1 + pager.c | 1 + parse-options.c | 1 + pathspec.c | 1 + pretty.c
[PATCH v2 4/6] config: don't implicitly use gitdir
Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
not set up) added a 'git_dir' field to the config_options struct. Let's
use this option field explicitly all the time instead of occasionally
falling back to calling 'git_pathdup("config")' to get the path to the
local repository configuration. This allows 'do_git_config_sequence()'
to not implicitly rely on global repository state.
Signed-off-by: Brandon Williams
---
builtin/config.c | 2 ++
config.c | 6 ++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 753c40a5c..f06a8dff2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,6 +539,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
+ if (!nongit)
+ config_options.git_dir = get_git_common_dir();
if (end_null) {
term = '\0';
diff --git a/config.c b/config.c
index 2390f98e3..4e2842689 100644
--- a/config.c
+++ b/config.c
@@ -219,8 +219,6 @@ static int include_by_gitdir(const struct config_options
*opts,
if (opts->git_dir)
git_dir = opts->git_dir;
- else if (have_git_dir())
- git_dir = get_git_dir();
else
goto done;
@@ -1548,8 +1546,6 @@ static int do_git_config_sequence(const struct
config_options *opts,
if (opts->git_dir)
repo_config = mkpathdup("%s/config", opts->git_dir);
- else if (have_git_dir())
- repo_config = git_pathdup("config");
else
repo_config = NULL;
@@ -1613,6 +1609,8 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
opts.respect_includes = 1;
+ if (have_git_dir())
+ opts.git_dir = get_git_common_dir();
if (git_config_with_options(fn, data, NULL, &opts) < 0)
/*
* git_config_with_options() normally returns only
--
2.13.1.518.g3df882009-goog
[PATCH v2 0/4] Improvements to sha1_file
Peff suggested putting in a new field in struct object_info for the object contents; I tried it and it seems to work out quite well. Patch 1 is unmodified from the previous version. Patches 2-3 have been rewritten, and patch 4 is similar except that the missing-lookup change is made to sha1_object_info_extended() instead of the now gone get_object(). As before, I would like review on patches 1-3 to go into the tree. (Patch 4 is a work in progress, and is here just to demonstrate the effectiveness of the refactoring.) Jonathan Tan (4): sha1_file: teach packed_object_info about typename sha1_file: move delta base cache code up sha1_file: consolidate storage-agnostic object fns sha1_file, fsck: add missing blob support Documentation/config.txt | 10 + builtin/fsck.c | 7 + cache.h | 13 ++ sha1_file.c | 506 +-- t/t3907-missing-blob.sh | 69 +++ 5 files changed, 418 insertions(+), 187 deletions(-) create mode 100755 t/t3907-missing-blob.sh -- 2.13.1.518.g3df882009-goog
[PATCH v2 3/4] sha1_file: consolidate storage-agnostic object fns
In sha1_file.c, there are a few functions that provide information on an
object regardless of its storage (cached, loose, or packed). Looking
through all non-static functions in sha1_file.c that take in an unsigned
char * pointer, the relevant ones are:
- sha1_object_info_extended
- sha1_object_info (auto-fixed by sha1_object_info_extended)
- read_sha1_file_extended (uses read_object)
- read_object_with_reference (auto-fixed by read_sha1_file_extended)
- has_sha1_file_with_flags
- assert_sha1_type (auto-fixed by sha1_object_info)
Looking at the 3 primary functions (sha1_object_info_extended,
read_object, has_sha1_file_with_flags), they independently implement
mechanisms such as object replacement, retrying the packed store after
failing to find the object in the packed store then the loose store, and
being able to mark a packed object as bad and then retrying the whole
process. Consolidating these mechanisms would be a great help to
maintainability.
Therefore, consolidate all 3 functions by extending
sha1_object_info_extended() to support the functionality needed by all 3
functions, and then modifying the other 2 to use
sha1_object_info_extended().
Note that has_sha1_file_with_flags() does not try cached storage,
whereas the other 2 functions do - this functionality is preserved.
Signed-off-by: Jonathan Tan
---
cache.h | 7 +++
sha1_file.c | 143 +++-
2 files changed, 81 insertions(+), 69 deletions(-)
diff --git a/cache.h b/cache.h
index 4d92aae0e..3c85867c3 100644
--- a/cache.h
+++ b/cache.h
@@ -1835,6 +1835,7 @@ struct object_info {
off_t *disk_sizep;
unsigned char *delta_base_sha1;
struct strbuf *typename;
+ void **contentp;
/* Response */
enum {
@@ -1866,6 +1867,12 @@ struct object_info {
*/
#define OBJECT_INFO_INIT {NULL}
+/*
+ * sha1_object_info_extended() supports the LOOKUP_ flags and the OBJECT_INFO_
+ * flags.
+ */
+#define OBJECT_INFO_QUICK 4
+#define OBJECT_INFO_SKIP_CACHED 8
extern int sha1_object_info_extended(const unsigned char *, struct object_info
*, unsigned flags);
extern int packed_object_info(struct packed_git *pack, off_t offset, struct
object_info *);
diff --git a/sha1_file.c b/sha1_file.c
index a158907d1..98086e21e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2005,19 +2005,6 @@ int parse_sha1_header(const char *hdr, unsigned long
*sizep)
return parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);
}
-static void *unpack_sha1_file(void *map, unsigned long mapsize, enum
object_type *type, unsigned long *size, const unsigned char *sha1)
-{
- int ret;
- git_zstream stream;
- char hdr[8192];
-
- ret = unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr));
- if (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)
- return NULL;
-
- return unpack_sha1_rest(&stream, hdr, *size, sha1);
-}
-
unsigned long get_size_from_delta(struct packed_git *p,
struct pack_window **w_curs,
off_t curpos)
@@ -2326,8 +2313,10 @@ static void *cache_or_unpack_entry(struct packed_git *p,
off_t base_offset,
if (!ent)
return unpack_entry(p, base_offset, type, base_size);
- *type = ent->type;
- *base_size = ent->size;
+ if (type)
+ *type = ent->type;
+ if (base_size)
+ *base_size = ent->size;
return xmemdupz(ent->data, ent->size);
}
@@ -2388,9 +2377,16 @@ int packed_object_info(struct packed_git *p, off_t
obj_offset,
* We always get the representation type, but only convert it to
* a "real" type later if the caller is interested.
*/
- type = unpack_object_header(p, &w_curs, &curpos, &size);
+ if (oi->contentp) {
+ *oi->contentp = cache_or_unpack_entry(p, obj_offset, oi->sizep,
+ &type);
+ if (!*oi->contentp)
+ type = OBJ_BAD;
+ } else {
+ type = unpack_object_header(p, &w_curs, &curpos, &size);
+ }
- if (oi->sizep) {
+ if (!oi->contentp && oi->sizep) {
if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
off_t tmp_pos = curpos;
off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
@@ -2685,8 +2681,10 @@ void *unpack_entry(struct packed_git *p, off_t
obj_offset,
free(external_base);
}
- *final_type = type;
- *final_size = size;
+ if (final_type)
+ *final_type = type;
+ if (final_size)
+ *final_size = size;
unuse_pack(&w_curs);
@@ -2920,6 +2918,7 @@ static int sha1_loose_object_info(const unsigned char
*sha1,
git_zstream stream;
char hdr[32];
struct strbuf hdrbuf = STRBUF_INIT;
+ un
[PATCH v2 1/4] sha1_file: teach packed_object_info about typename
In commit 46f0344 ("sha1_file: support reading from a loose object of
unknown type", 2015-05-06), "struct object_info" gained a "typename"
field that could represent a type name from a loose object file, whether
valid or invalid, as opposed to the existing "typep" which could only
represent valid types. Some relatively complex manipulations were added
to avoid breaking packed_object_info() without modifying it, but it is
much easier to just teach packed_object_info() about the new field.
Therefore, teach packed_object_info() as described above.
Signed-off-by: Jonathan Tan
---
sha1_file.c | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 59a4ed2ed..a52b27541 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2277,9 +2277,18 @@ int packed_object_info(struct packed_git *p, off_t
obj_offset,
*oi->disk_sizep = revidx[1].offset - obj_offset;
}
- if (oi->typep) {
- *oi->typep = packed_to_object_type(p, obj_offset, type,
&w_curs, curpos);
- if (*oi->typep < 0) {
+ if (oi->typep || oi->typename) {
+ enum object_type ptot;
+ ptot = packed_to_object_type(p, obj_offset, type, &w_curs,
+curpos);
+ if (oi->typep)
+ *oi->typep = ptot;
+ if (oi->typename) {
+ const char *tn = typename(ptot);
+ if (tn)
+ strbuf_addstr(oi->typename, tn);
+ }
+ if (ptot < 0) {
type = OBJ_BAD;
goto out;
}
@@ -2960,7 +2969,6 @@ int sha1_object_info_extended(const unsigned char *sha1,
struct object_info *oi,
struct cached_object *co;
struct pack_entry e;
int rtype;
- enum object_type real_type;
const unsigned char *real = lookup_replace_object_extended(sha1, flags);
co = find_cached_object(real);
@@ -2992,18 +3000,9 @@ int sha1_object_info_extended(const unsigned char *sha1,
struct object_info *oi,
return -1;
}
- /*
-* packed_object_info() does not follow the delta chain to
-* find out the real type, unless it is given oi->typep.
-*/
- if (oi->typename && !oi->typep)
- oi->typep = &real_type;
-
rtype = packed_object_info(e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real);
- if (oi->typep == &real_type)
- oi->typep = NULL;
return sha1_object_info_extended(real, oi, 0);
} else if (in_delta_base_cache(e.p, e.offset)) {
oi->whence = OI_DBCACHED;
@@ -3014,10 +3013,6 @@ int sha1_object_info_extended(const unsigned char *sha1,
struct object_info *oi,
oi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
rtype == OBJ_OFS_DELTA);
}
- if (oi->typename)
- strbuf_addstr(oi->typename, typename(*oi->typep));
- if (oi->typep == &real_type)
- oi->typep = NULL;
return 0;
}
--
2.13.1.518.g3df882009-goog
[PATCH v2 2/4] sha1_file: move delta base cache code up
In a subsequent patch, packed_object_info() will be modified to use the
delta base cache, so move the relevant code to before
packed_object_info().
Signed-off-by: Jonathan Tan
---
sha1_file.c | 226 +++-
1 file changed, 116 insertions(+), 110 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index a52b27541..a158907d1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2239,116 +2239,6 @@ static enum object_type packed_to_object_type(struct
packed_git *p,
goto out;
}
-int packed_object_info(struct packed_git *p, off_t obj_offset,
- struct object_info *oi)
-{
- struct pack_window *w_curs = NULL;
- unsigned long size;
- off_t curpos = obj_offset;
- enum object_type type;
-
- /*
-* We always get the representation type, but only convert it to
-* a "real" type later if the caller is interested.
-*/
- type = unpack_object_header(p, &w_curs, &curpos, &size);
-
- if (oi->sizep) {
- if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
- off_t tmp_pos = curpos;
- off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
- type, obj_offset);
- if (!base_offset) {
- type = OBJ_BAD;
- goto out;
- }
- *oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);
- if (*oi->sizep == 0) {
- type = OBJ_BAD;
- goto out;
- }
- } else {
- *oi->sizep = size;
- }
- }
-
- if (oi->disk_sizep) {
- struct revindex_entry *revidx = find_pack_revindex(p,
obj_offset);
- *oi->disk_sizep = revidx[1].offset - obj_offset;
- }
-
- if (oi->typep || oi->typename) {
- enum object_type ptot;
- ptot = packed_to_object_type(p, obj_offset, type, &w_curs,
-curpos);
- if (oi->typep)
- *oi->typep = ptot;
- if (oi->typename) {
- const char *tn = typename(ptot);
- if (tn)
- strbuf_addstr(oi->typename, tn);
- }
- if (ptot < 0) {
- type = OBJ_BAD;
- goto out;
- }
- }
-
- if (oi->delta_base_sha1) {
- if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
- const unsigned char *base;
-
- base = get_delta_base_sha1(p, &w_curs, curpos,
- type, obj_offset);
- if (!base) {
- type = OBJ_BAD;
- goto out;
- }
-
- hashcpy(oi->delta_base_sha1, base);
- } else
- hashclr(oi->delta_base_sha1);
- }
-
-out:
- unuse_pack(&w_curs);
- return type;
-}
-
-static void *unpack_compressed_entry(struct packed_git *p,
- struct pack_window **w_curs,
- off_t curpos,
- unsigned long size)
-{
- int st;
- git_zstream stream;
- unsigned char *buffer, *in;
-
- buffer = xmallocz_gently(size);
- if (!buffer)
- return NULL;
- memset(&stream, 0, sizeof(stream));
- stream.next_out = buffer;
- stream.avail_out = size + 1;
-
- git_inflate_init(&stream);
- do {
- in = use_pack(p, w_curs, curpos, &stream.avail_in);
- stream.next_in = in;
- st = git_inflate(&stream, Z_FINISH);
- if (!stream.avail_out)
- break; /* the payload is larger than it should be */
- curpos += stream.next_in - in;
- } while (st == Z_OK || st == Z_BUF_ERROR);
- git_inflate_end(&stream);
- if ((st != Z_STREAM_END) || stream.total_out != size) {
- free(buffer);
- return NULL;
- }
-
- return buffer;
-}
-
static struct hashmap delta_base_cache;
static size_t delta_base_cached;
@@ -2486,6 +2376,122 @@ static void add_delta_base_cache(struct packed_git *p,
off_t base_offset,
hashmap_add(&delta_base_cache, ent);
}
+int packed_object_info(struct packed_git *p, off_t obj_offset,
+ struct object_info *oi)
+{
+ struct pack_window *w_curs = NULL;
+ unsigned long size;
+ off_t curpos = obj_offset;
+ enum object_type type;
+
+ /*
+* We always get the representation type, but only convert i
[PATCH v2 4/4] sha1_file, fsck: add missing blob support
Currently, Git does not support repos with very large numbers of blobs or repos that wish to minimize manipulation of certain blobs (for example, because they are very large) very well, even if the user operates mostly on part of the repo, because Git is designed on the assumption that every blob referenced by a tree object is available somewhere in the repo storage. As a first step to reducing this problem, add rudimentary support for missing blobs by teaching sha1_file to invoke a hook whenever a blob is requested and unavailable but registered to be missing, and by updating fsck to tolerate such blobs. The hook is a shell command that can be configured through "git config"; this hook takes in a list of hashes and writes (if successful) the corresponding objects to the repo's local storage. This commit does not include support for generating such a repo; neither has any command (other than fsck) been modified to either tolerate missing blobs (without invoking the hook) or be more efficient in invoking the missing blob hook. Only a fallback is provided in the form of sha1_file invoking the missing blob hook when necessary. In order to determine the code changes in sha1_file.c necessary, I investigated the following: (1) functions in sha1_file that take in a hash, without the user regarding how the object is stored (loose or packed) (2) functions in sha1_file that operate on packed objects (because I need to check callers that know about the loose/packed distinction and operate on both differently, and ensure that they can handle the concept of objects that are neither loose nor packed) (1) is handled by the modification to sha1_object_info_extended(). For (2), I looked through the same functions as in (1) and also for_each_packed_object. The ones that are relevant are: - parse_pack_index - http - indirectly from http_get_info_packs - find_pack_entry_one - this searches a single pack that is provided as an argument; the caller already knows (through other means) that the sought object is in a specific pack - find_sha1_pack - fast-import - appears to be an optimization to not store a file if it is already in a pack - http-walker - to search through a struct alt_base - http-push - to search through remote packs - has_sha1_pack - builtin/fsck - fixed in this commit - builtin/count-objects - informational purposes only (check if loose object is also packed) - builtin/prune-packed - check if object to be pruned is packed (if not, don't prune it) - revision - used to exclude packed objects if requested by user - diff - just for optimization - for_each_packed_object - reachable - only to find recent objects - builtin/fsck - fixed in this commit - builtin/cat-file - see below As described in the list above, builtin/fsck has been updated. I have left builtin/cat-file alone; this means that cat-file --batch-all-objects will only operate on objects physically in the repo. An alternative design that I considered but rejected: - Adding a hook whenever a packed blob is requested, not on any blob. That is, whenever we attempt to search the packfiles for a blob, if it is missing (from the packfiles and from the loose object storage), to invoke the hook (which must then store it as a packfile), open the packfile the hook generated, and report that the blob is found in that new packfile. This reduces the amount of analysis needed (in that we only need to look at how packed blobs are handled), but requires that the hook generate packfiles (or for sha1_file to pack whatever loose objects are generated), creating one packfile for each missing blob and potentially very many packfiles that must be linearly searched. This may be tolerable now for repos that only have a few missing blobs (for example, repos that only want to exclude large blobs), and might be tolerable in the future if we have batching support for the most commonly used commands, but is not tolerable now for repos that exclude a large amount of blobs. Signed-off-by: Jonathan Tan --- Documentation/config.txt | 10 +++ builtin/fsck.c | 7 +++ cache.h | 6 ++ sha1_file.c | 156 ++- t/t3907-missing-blob.sh | 69 + 5 files changed, 233 insertions(+), 15 deletions(-) create mode 100755 t/t3907-missing-blob.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index dd4beec39..10da5fde1 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -390,6 +390,16 @@ The default is false, except linkgit:git-clone[1] or linkgit:git-init[1] will probe and set core.ignoreCase true if appropriate when the repository is created. +core.missingBlobCommand:: + If set, whenever a blob in the local repo is attempted to be + read but is missing, invoke this shell command to generate or +
