HOPETO HEAR FROM YOU
Hello dear how are you? Nice to meet you,my name is Miss Fatima, can we become friends? hope to hear from you so that we can know each other very well,love matters mostly in life,i will also send you my pictures and tell you more about myself, my email address is, fatima.ab...@yahoo.com waiting to hear from you soon. Miss.Fatima
Clone repository computer A to remote B doenst work
Hello all, I'm trying clone in windows a git repository to other remote machine (NAS Linux based). I have installed git for windows but i didn't installed nothing in the other remote machine (NAS Linux based). I open git-bash and I try do this: "git clone user@IP:/directory/where/i/want/clone". This doesnt work because I obtain "sh: git-upload-pack: command not found". I search in google and I found a possible solution, I need add -u /path/to/git-upload-pack. When i try again writing git clone -u /c/Git/mingw64/bin/git-upload-pack user@IP:/directory/where/i/want/clone After I Write the password I obtain sh: C:/Git/mingw64/bin/git-upload-pack: No such file or directory. But it exists! (in my local computer windows) I'm newbie with Git and Unix system. What can i do ? Thanks a lot!
Re: [PATCH] checkout: include the checkout.h header file
On 11/24, Ramsay Jones wrote:
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Thomas,
>
> If you need to re-roll your 'tg/worktree-create-tracking' branch, could
> you please squash this into the relevant patch (commit 6736ae9593,
> "checkout: factor out functions to new lib file", 2017-11-22).
>
> [noticed by sparse]
Thanks for noticing this, I'll squash it into my re-roll.
> Thanks!
>
> ATB,
> Ramsay Jones
>
> checkout.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/checkout.c b/checkout.c
> index b0c744d37..ac42630f7 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "remote.h"
> +#include "checkout.h"
>
> struct tracking_name_data {
> /* const */ char *src_ref;
> --
> 2.15.0
Xmas Loan Bonaza
Are You In Need Of A Private Or Business Loans At 2% Rate For Various Purposes? If Yes; Contact us with this info below. Full Name: Amount Needed: Duration: Country Cell No: Sex: Best Regards
RE: Clone repository computer A to remote B doenst work
On November 25, 2017 4:31 AM Roberto Garcia wrote: >I'm trying clone in windows a git repository to other remote machine (NAS >Linux based). >I have installed git for windows but i didn't installed nothing in the other >remote machine (NAS Linux based). You have two choices: 1. Install git on your LINYX machine, which you probably can't do if it's a pure NAS outside of your control. 2. Run everything off Windows as git in local mode. Mount the NAS as a windows drive. In a command terminal: a. cd X:\Share\repo.git #you'll have to mkdir this b. git init --bare #creates a new empty repo on your NAS c. cd C:\MyStuff #where you keep your clones d. git clone -l X:\Share\repo.git #clone the bare repository e. Start adding stuff (git add, git commit) f. git push # to move commits to your NAS repo. Then you have your relationship and can push/pull from your NAS entirely from within Windows executing objects. Change directories and drive letters accordingly. -l means local, so git won't be starting any git-upload-pack processes remotely. Variations on this should work. Good luck. Randall
Re: [PATCH v4 2/4] worktree: add --[no-]track option to the add subcommand
On 11/24, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > index b5c47ac602..53042ce565 100755
> > --- a/t/t2025-worktree-add.sh
> > +++ b/t/t2025-worktree-add.sh
> > @@ -313,5 +313,60 @@ test_expect_success 'checkout a branch under bisect' '
> > test_expect_success 'rename a branch under bisect not allowed' '
> > test_must_fail git branch -M under-bisect bisect-with-new-name
> > '
> > +# Is branch "refs/heads/$1" set to pull from "$2/$3"?
> > +test_branch_upstream () {
> > + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream &&
> > + {
> > + git config "branch.$1.remote" &&
> > + git config "branch.$1.merge"
> > + } >actual.upstream &&
> > + test_cmp expect.upstream actual.upstream
> > +}
>
> OK.
>
> > +test_expect_success '--track sets up tracking' '
> > + test_when_finished rm -rf track &&
> > + git worktree add --track -b track track master &&
> > + git config "branch.track.merge" &&
> > + (
> > + test_branch_upstream track . master
> > + )
> > +'
>
> Is this "git config" necessary, or is it a remnant of a debugging
> session? It is tested in the helper that branch.track.merge is set
> to something, and otherwise the helper would fail the same way as
> this standalnoe "git config" would, no?
It's a remnant of a debugging session, sorry. It would indeed fail in
the same way, so just leaving the 'test_branch_upstream' is enough.
Also looking at that, there's no need for it to be in a subshell, will
fix that as well.
> > +# setup remote repository $1 and repository $2 with $1 set up as
> > +# remote. The remote has two branches, master and foo.
> > +setup_remote_repo () {
> > + git init $1 &&
> > + (
> > + cd $1 &&
> > + test_commit $1_master &&
> > + git checkout -b foo &&
> > + test_commit upstream_foo
> > + ) &&
> > + git init $2 &&
> > + (
> > + cd $2 &&
> > + test_commit $2_master &&
> > + git remote add $1 ../$1 &&
> > + git config remote.$1.fetch \
> > + "refs/heads/*:refs/remotes/$1/*" &&
> > + git fetch --all
> > + )
> > +}
> > +
> > +test_expect_success '--no-track avoids setting up tracking' '
> > + test_when_finished rm -rf repo_upstream repo_local foo &&
> > + setup_remote_repo repo_upstream repo_local &&
> > + (
> > + cd repo_local &&
> > + git worktree add --no-track -b foo ../foo repo_upstream/foo
> > + ) &&
> > + (
> > + cd foo &&
> > + ! test_branch_upstream foo repo_upstream foo &&
>
> It is true that this test helper must yield failure. But what you
> expect probably is more than that, no? For example, the test helper
> would fail even if branch.foo.remote is set to the upstream as long
> as branch.foo.merge is not set to point at their foo, but what you
> really want to make sure is that neither configuration variable is
> set.
Yeah you're right, this test is a bit too loose. Will fix that in the
re-roll. Thanks!
> > + git rev-parse repo_upstream/foo >expect &&
> > + git rev-parse foo >actual &&
> > + test_cmp expect actual
> > + )
> > +'
> >
> > test_done
[PATCH] RelNotes: minor typo fixes in 2.15.1 draft
Signed-off-by: Todd Zullinger
---
Documentation/RelNotes/2.15.1.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/RelNotes/2.15.1.txt
b/Documentation/RelNotes/2.15.1.txt
index 47f23b56fe..81dd64b4ad 100644
--- a/Documentation/RelNotes/2.15.1.txt
+++ b/Documentation/RelNotes/2.15.1.txt
@@ -13,7 +13,7 @@ Fixes since v2.15
latter, which has been fixed.
* The experimental "color moved lines differently in diff output"
- feature was buggy around "ignore whitespace changes" edges, whihch
+ feature was buggy around "ignore whitespace changes" edges, which
has been corrected.
* Instead of using custom line comparison and hashing functions to
@@ -24,7 +24,7 @@ Fixes since v2.15
HEAD points at, which have been fixed.
* "git commit", after making a commit, did not check for errors when
- asking on what branch it made the commit, which has been correted.
+ asking on what branch it made the commit, which has been corrected.
* "git status --ignored -u" did not stop at a working tree of a
separate project that is embedded in an ignored directory and
@@ -35,7 +35,7 @@ Fixes since v2.15
--recurse-submodules" has been fixed.
* A recent regression in "git rebase -i" that broke execution of git
- commands from subdirectories via "exec" insn has been fixed.
+ commands from subdirectories via "exec" instruction has been fixed.
* "git check-ref-format --branch @{-1}" bit a "BUG()" when run
outside a repository for obvious reasons; clarify the documentation
--
2.15.0
[PATCH] RelNotes: minor typo fixes in 2.15.1 draft
Signed-off-by: Todd Zullinger
---
Documentation/RelNotes/2.15.1.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/RelNotes/2.15.1.txt
b/Documentation/RelNotes/2.15.1.txt
index 47f23b56fe..81dd64b4ad 100644
--- a/Documentation/RelNotes/2.15.1.txt
+++ b/Documentation/RelNotes/2.15.1.txt
@@ -13,7 +13,7 @@ Fixes since v2.15
latter, which has been fixed.
* The experimental "color moved lines differently in diff output"
- feature was buggy around "ignore whitespace changes" edges, whihch
+ feature was buggy around "ignore whitespace changes" edges, which
has been corrected.
* Instead of using custom line comparison and hashing functions to
@@ -24,7 +24,7 @@ Fixes since v2.15
HEAD points at, which have been fixed.
* "git commit", after making a commit, did not check for errors when
- asking on what branch it made the commit, which has been correted.
+ asking on what branch it made the commit, which has been corrected.
* "git status --ignored -u" did not stop at a working tree of a
separate project that is embedded in an ignored directory and
@@ -35,7 +35,7 @@ Fixes since v2.15
--recurse-submodules" has been fixed.
* A recent regression in "git rebase -i" that broke execution of git
- commands from subdirectories via "exec" insn has been fixed.
+ commands from subdirectories via "exec" instruction has been fixed.
* "git check-ref-format --branch @{-1}" bit a "BUG()" when run
outside a repository for obvious reasons; clarify the documentation
--
2.15.0
Re: [PATCH v4 4/4] worktree: make add dwim
On 11/24, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > Currently 'git worktree add ' creates a new branch named after the
> > basename of the , that matches the HEAD of whichever worktree we
> > were on when calling "git worktree add ".
> >
> > Make 'git worktree add behave more like the dwim machinery in
> > 'git checkout ', i.e. check if the new branch name uniquely
> > matches the branch name of a remote tracking branch, and if so check out
> > that branch and set the upstream to the remote tracking branch.
> >
> > This is a change of behaviour compared to the current behaviour, where
> > we create a new branch matching HEAD. However as 'git worktree' is
> > still an experimental feature, and it's easy to notice/correct the
> > behaviour in case it's not what the user desired it's probably okay to
> > break existing behaviour here.
>
> Is it "easy to notice"? I doubt it. Even if you assume that
> everybody uses bash prompt that shows the name of the branch, the
> user sees the same name of the branch in either mode.
With "easy" I meant at creation time, looking at the output of 'git
worktree add', which with the new version shows that the the new
branch has been set up to track the remote branch, and also shows the
commit HEAD now points to.
This would be the output in the new version:
$ git worktree add ../bla
Branch 'bla' set up to track remote branch 'bla' from 'origin'.
Preparing ../bla (identifier bla)
HEAD is now at 4aade43 bla
vs. the output without the changed behaviour:
$ git worktree add ../bla
Preparing ../bla (identifier bla)
HEAD is now at 0f215c9 initial import
Of course that assumes that it's used directly, not in scripts, and
that users will actually read the output of the command when they
invoke it. Maybe these are not safe assumptions to make though, and
we'd rather not have this on by default then. As I mentioned
previously I would prefer having this as default, but I'm happy to
hide this behaviour behind a flag if we want to be more careful about
introducing this. Dunno?
> > In order to also satisfy users who want the current behaviour of
> > creating a new branch from HEAD, add a '--no-track' flag, which disables
> > the new behaviour, and keeps the old behaviour of creating a new branch
> > from the head of the current worktree.
>
> I am not sure if this is a good match for "--track/--no-track";
> which branch is to be checked out (either "automatically from the
> unique remote-tracking branch" or "the current one") is one choice,
> and whether the resulting branch is marked explicitly as integrating
> with the remote or not is another choice within one branch of the
> first choice. IOW, this makes it impossible to say "create the branch
> based on the unique remote-tracking branch, but do not add the two
> branch.*.{merge,remote} variables".
Hmm good point. Maybe we'll need another flag for this. Maybe
--[no-]guess-remote would work, and a corresponding
worktree.guessRemote config would work? That's the best I could come
up with, better suggestions are definitely welcome.
> Also, you have several mention of "remote tracking branch" in these
> patches. Please consistently spell them as "remote-tracking branch"
> to be consistent with Documentation/glossary-content.txt and avoid a
> casual/careful reference to "tracking branch" if possible, unless it
> is quite clear to the readers that you are being loose for the sake
> of brevity. Some people used "tracking branch" to mean the local
> branch that is marked as the branch to integrate with the work on
> a branch at a remote that caused user confusion in the past.
I must admit I wasn't aware of Documentation/glossary-content.txt and
have seen "tracking branch" in other places, so I was just repeating
the pattern.
> That is
>
> refs/remotes/origin/topic is a remote-tracking branch for the
> branch 'topic' that came from the 'origin' remote.
>
> when you have branch.foo.remote=origin and
> branch.foo.merge=refs/heads/topic, then your local branch foo is
> marked to integrate with the 'topic' branch at the 'origin'
> remote.
>
> and these two are quite different things that people in the past and
> over time loosely used a phrase "tracking branch" to cause confusion.
Thanks for the clarification, will fix in the re-roll.
Re: [PATCH] RelNotes: minor typo fixes in 2.15.1 draft
Apologies for the duplicate. I used the `-n` option, mistakenly thinking it was a synonym for `--dry-run` and didn't pay enough attention to see that it sent. (The only indication is s/OK./Dry &/ which I missed.) It was mildly surprising that the script didn't warn or complain about an unknown option. After a quick look, that seems to be due to the Getopt::Long pass_through option which sends unknown options to format-patch. That doesn't remove the user-error on my part, of course. ;) -- Todd ~~ Cogito cogito ergo cogito sum -- I think that I think, therefore I think that I am. -- Ambrose Bierce, "The Devil's Dictionary"
Re: [PATCH v4 4/4] worktree: make add dwim
On Sat, 2017-11-25 at 17:50 +, Thomas Gummerer wrote: > This would be the output in the new version: > > $ git worktree add ../bla > Branch 'bla' set up to track remote branch 'bla' from 'origin'. > Preparing ../bla (identifier bla) > HEAD is now at 4aade43 bla > > vs. the output without the changed behaviour: > > $ git worktree add ../bla > Preparing ../bla (identifier bla) > HEAD is now at 0f215c9 initial import > > Of course that assumes that it's used directly, not in scripts, and > that users will actually read the output of the command when they > invoke it. Maybe these are not safe assumptions to make though, and > we'd rather not have this on by default then. As I mentioned > previously I would prefer having this as default, but I'm happy to > hide this behaviour behind a flag if we want to be more careful about > introducing this. Dunno? Speaking as a simple user, I find the current behavior of Git worktree add very frustrating; I am constantly wanting to create worktrees for other peoples' branches so I can look at the code there without messing up my workspace, and it's really inconvenient to do that now. Also, the current special handling of the directory name as a putative branch name is not helpful for me because many of the branches I need to examine use "/" as their separator. I don't begrudge making that feature more "DWIM" for those that can use it, but hopefully some help is forthcoming for those who can't. For example, I need to create a local worktree for the remote rel/1.0 branch... what do I do? What I want to work is this: git worktree add ../1.0 rel/1.0 and have it create a worktree at ../1.0, then do the equivalent of "git checkout rel/1.0" which includes setting up to track the remote branch. But of course this doesn't work at all; I get: fatal: invalid reference: rel/1.0 Personally I would think it odd to have to add an extra flag to get what I would expect would be "normal" behavior (checkout). But maybe that's just me.
[PATCH] submodule--helper.c: i18n: add a missing space in message
The message spans over 2 lines but the C conconcatenation does not add
the needed space between the two lines.
---
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2086f0eb0..a5c4a8a69 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -623,7 +623,7 @@ static void status_submodule(const char *path, const struct
object_id *ce_oid,
if (refs_head_ref(get_submodule_ref_store(path),
handle_submodule_head_ref, &oid))
- die(_("could not resolve HEAD ref inside the"
+ die(_("could not resolve HEAD ref inside the "
"submodule '%s'"), path);
print_status(flags, '+', path, &oid, displaypath);
--
2.15.0
[PATCH] submodule--helper.c: i18n: add a missing space in message
The message spans over 2 lines but the C conconcatenation does not add
the needed space between the two lines.
Signed-off-by: Jean-Noel Avila
---
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2086f0eb0..a5c4a8a69 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -623,7 +623,7 @@ static void status_submodule(const char *path, const struct
object_id *ce_oid,
if (refs_head_ref(get_submodule_ref_store(path),
handle_submodule_head_ref, &oid))
- die(_("could not resolve HEAD ref inside the"
+ die(_("could not resolve HEAD ref inside the "
"submodule '%s'"), path);
print_status(flags, '+', path, &oid, displaypath);
--
2.15.0
Re: [PATCH v4 4/4] worktree: make add dwim
On 11/25, Paul Smith wrote: > On Sat, 2017-11-25 at 17:50 +, Thomas Gummerer wrote: > > This would be the output in the new version: > > > > $ git worktree add ../bla > > Branch 'bla' set up to track remote branch 'bla' from 'origin'. > > Preparing ../bla (identifier bla) > > HEAD is now at 4aade43 bla > > > > vs. the output without the changed behaviour: > > > > $ git worktree add ../bla > > Preparing ../bla (identifier bla) > > HEAD is now at 0f215c9 initial import > > > > Of course that assumes that it's used directly, not in scripts, and > > that users will actually read the output of the command when they > > invoke it. Maybe these are not safe assumptions to make though, and > > we'd rather not have this on by default then. As I mentioned > > previously I would prefer having this as default, but I'm happy to > > hide this behaviour behind a flag if we want to be more careful about > > introducing this. Dunno? > > Speaking as a simple user, I find the current behavior of Git worktree > add very frustrating; I am constantly wanting to create worktrees for > other peoples' branches so I can look at the code there without messing > up my workspace, and it's really inconvenient to do that now. > > Also, the current special handling of the directory name as a putative > branch name is not helpful for me because many of the branches I need > to examine use "/" as their separator. I don't begrudge making that > feature more "DWIM" for those that can use it, but hopefully some help > is forthcoming for those who can't. > > For example, I need to create a local worktree for the remote rel/1.0 > branch... what do I do? > > What I want to work is this: > > git worktree add ../1.0 rel/1.0 > > and have it create a worktree at ../1.0, then do the equivalent of "git > checkout rel/1.0" which includes setting up to track the remote branch. > But of course this doesn't work at all; I get: > > fatal: invalid reference: rel/1.0 > > Personally I would think it odd to have to add an extra flag to get > what I would expect would be "normal" behavior (checkout). > > But maybe that's just me. This part is getting done in 3/4, and is definitely going to work without an additional flag, so this is (hopefully) soon going to work just as you want :) This is less controversial because as you mentioned this currently doesn't work at all, so there are no backwards compatibility worries. For the other case of git worktree add ../foo however we currently document one behaviour, which I would like to change (I usually have branches without a / in that I want to look at) we currently document one behaviour, which I'd like to change. So in that case we are a bit worried about backwards compatibility, and how this will affect current users that have a certain expectation of how the command is supposed to work, hence the discussion of whether to hide the new behaviour behind a flag or not. Either way, if we do put the behaviour behind a flag, I'll also add a configuration variable, which can be set to enable the new behaviour so one doesn't have to type out the flag all the time.
git-p4: cloning with a change number does not import all files
Hi, I created a git repository with these commands: git p4 clone //perforce/path@123456 repo cd repo git p4 rebase Some files created before the change 123456 do not exist in git history. I do see why, but those files were not modified after that change number. If I use "git p4 sync --detect-branches" instead of "git p4 rebase", the branch contains all the files, but not the main trunk, and they appear to be added in the first commit of the branch. To avoid the problem, I must clone with "@all" or with the change number when //perforce/path was created, which is significantly longer. Regards, P. Rouleau
RE: [PATCH v4 4/4] worktree: make add dwim
On November 25, 2017 3:06 PM Thomas Gummerer wrote: >however we currently document one behaviour, which I would like to change (I usually have branches >without a / in that I want to look at) we currently document one behaviour, which I'd like to change. So >in that case we are a bit worried about backwards compatibility, and how this will affect current users >that have a certain expectation of how the command is supposed to work, hence the discussion of >whether to hide the new behaviour behind a flag or not. >Either way, if we do put the behaviour behind a flag, I'll also add a configuration variable, which can >be set to enable the new behaviour so one doesn't have to type out the flag all the time. To be consistent with other commands, you could put path after -- and the ambiguity with refs containing '/' goes away, as refs before the -- would always be considered refs while after you have paths. What I don't like is the current add syntax of: git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] [] where the path-spec precedes branch making things a bit icky. It might be better to have an alternate syntax of: git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] [] git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] [] -- But since we only have one path, that may be redundant. Just a thought, as -- avoids a lot of interpretation evils. While we're here, I wonder whether should be changed to for more general use. Consider release identifiers using tags, and using the tag instead of branch to define commit on which the worktree is based. Cheers, Randall -- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(2112884442) -- In my real life, I talk too much.
Re: [PATCH] Makefile: check that tcl/tk is installed
On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder wrote: > By default running `make install` in the root directory of the > project will set TCLTK_PATH to `wish` and then go into the "git-gui" > and "gitk-git" sub-directories to build and install these 2 > sub-projects. Has this patch fallen through the cracks or is there an unresolved issue?
Re: [PATCH v4 4/4] worktree: make add dwim
On 11/25, Randall S. Becker wrote: > On November 25, 2017 3:06 PM Thomas Gummerer wrote: > > >however we currently document one behaviour, which I would like to change > >(I usually have branches > >without a / in that I want to look at) we currently document one behaviour, > >which I'd like to change. So > >in that case we are a bit worried about backwards compatibility, and how > >this will affect current users > >that have a certain expectation of how the command is supposed to work, > >hence the discussion of > >whether to hide the new behaviour behind a flag or not. > > >Either way, if we do put the behaviour behind a flag, I'll also add a > >configuration variable, which can > >be set to enable the new behaviour so one doesn't have to type out the flag > >all the time. > > To be consistent with other commands, you could put path after -- and the > ambiguity with refs containing '/' goes away, as refs before the -- would > always be considered refs while after you have paths. > > What I don't like is the current add syntax of: > git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] > [] > > where the path-spec precedes branch making things a bit icky. It might be > better to have an alternate syntax of: > > git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] > [] > git worktree add [-f] [--detach] [--checkout] [--lock] [-b ] > [] -- Hmm I don't think there is any ambiguity there, the first argument always needs to be a path, and the second one is always a commit-ish. So this way there is no disambiguation needed. I'm not convinced the alternative syntax would buy us much, at least not in the context of what this series is trying to do. > But since we only have one path, that may be redundant. Just a thought, as > -- avoids a lot of interpretation evils. While we're here, I wonder whether > should be changed to for more general use. Consider > release identifiers using tags, and using the tag instead of branch to > define commit on which the worktree is based. 'git worktree add' can already take a commit-ish, it's just not documented that way. I'll add a patch updating the documentation to the series. > Cheers, > Randall > > -- Brief whoami: NonStop&UNIX developer since approximately > UNIX(421664400)/NonStop(2112884442) > -- In my real life, I talk too much. > > > > >
Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875
Thanks for the review. I saw only reaction of the Jeff in the original thread and though that it is ok otherwise. I'm fixing the things you mentioned. On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote: >> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char >> **out) > > Wrong data type: s/size_t req_len/ssize_t req_len/ Passing negative value to the function makes no sense. I could add explicit type cast to make it clear. It should be safe as site_t's range is bigger, and overflown CONTENT_LENGTH results in die() at parsing (I have a test which verifies it) > Rather than writing an entirely new "read" function, how about just > modifying the existing read_request() to optionally limit the read to > a specified number of bytes? I'll check it a bit separately. -- Max
Re: git status always modifies index?
Hi Peff, On Wed, 22 Nov 2017, Jeff King wrote: > On Wed, Nov 22, 2017 at 01:56:35PM -0800, Jonathan Nieder wrote: > > > Jeff King wrote: > > > On Wed, Nov 22, 2017 at 12:27:20PM -0800, Jonathan Nieder wrote: > > > > >> That said, I wonder if this use case is an illustration that a name > > >> like --no-lock-index (as was used in Git for Windows when this feature > > >> first appeared) or --no-refresh-on-disk-index (sorry, I am terrible at > > >> coming up with option names) would make the feature easier to > > >> discover. > > [...] > > > Or maybe just living with the minor philosophical rough edges, > > > since it seems OK in practice. > > > > To be clear, my concern is not philosophical but practical: I'm saying > > if it's a "git status" option (or at least shows up in the "git > > status" manpage) and it is memorably about $GIT_DIR/index (at least > > mentions that in its description) then it is more likely to help > > people. > > Right, I went a little off track of your original point. > > What I was trying to get at is that naming it "status --no-lock-index" > would not be the same thing (even though with the current implementation > it would behave the same). IOW, can we improve the documentation of > "status" to point to make it easier to discover this use case. I had the hunch that renaming the option (and moving it away from `git status`, even if it is currently only affecting `git status` and even if it will most likely be desirable to have the option to really only prevent `git status` from writing .lock files) was an unfortunate decision (and made my life as Git for Windows quite a bit harder than really necessary, it cost me over one workday of a bug hunt, mainly due to a false flag indicating `git rebase` to be the culprit). And I hinted at it, too. Maybe I should trust my instincts and act on them more. It is not like it was the first time that I had doubts that turned out to have merit, see e.g. the regression introduced into the formerly rock-solid set_hidden_flag() patches due to changes I made reluctantly during upstreaming, or the regression introduced during v1->v2 in my regex-buf patches that caused problems with mulibc and AIX. I really never understood why --no-optional-locks had to be introduced when it did exactly the same as --no-lock-index, and when the latter has a right to exist in the first place, even in the purely hypothetical case that we teach --no-optional-locks to handle more cases than just `git status`' writing of the index (and in essence, it looks like premature optimization): it is a very concrete use case that a user may want `git status` to refrain from even trying to write any file, as this thread shows very eloquently. Maybe it is time to reintroduce --no-lock-index, and make --no-optional-locks' functionality a true superset of --no-lock-index'. At least that is what my gut feeling tells me should be done. Ciao, Dscho
Re: Problem installing Git
Hi Igor, On Thu, 23 Nov 2017, Igor Djordjevic wrote: > [ +Cc: Git for Windows mailing list ] I have no idea why it claimed that that group does not exist, the email address looks correct to me. > On 23/11/2017 19:51, Phil Martel wrote: > > I'm trying to install Git-2.15.0-64-bit.exe onto my Windows 10 > > machine. When I run this installer program no matter what options I > > try or whether I run as administrator it ends up with an error box > > saying "The drive or UNC share you selected does not exist or is not > > accessible. Please select another". I do not see any way of > > selecting a drive. Any suggestions? > > From what I could Google around, this seems to be (Inno Setup?) > installation related issue...? Indeed. > Do you already have "Git for Windows" installed? If so, does it work > if you try uninstalling it first? That is a workaround, correct. > p.s. Note the existence of "Git for Windows"[1] specific mailing list > as well, where this issue might belong better. > > [1] git-for-wind...@googlegroups.com I think a much better place is the Git for Windows bug tracker (if you ever wonder where the bug tracker is, or the home page, or the repository or the FAQ, there are links in the upper left of the release notes -- which probably nobody reads, even if I really try to make them worth the while -- and which you can find in C:\Program Files\Git\ReleaseNotes.html if you closed the tab after installing Git for Windows). And indeed, there is already a ticket for this issue: https://github.com/git-for-windows/git/issues/1074 The original reporter did not respond to any questions, maybe you can do better, Phil? Ciao, Johannes
Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals
On Fri, Nov 24, 2017 at 7:29 PM, Junio C Hamano wrote: > Elijah Newren writes: > >> But what it really is forced to do is more of a 4-way merge; a good >> chunk of its annoying complexity is based around this (undocumented >> and unfortunate) reality. It derives from what I consider a simple >> design flaw. > > Yes, and it does not help that it wants to write into the filesystem > while it performs the outermost merges. > > In the ideal world, we should > > - ask unpack_trees() to do "read-tree -m" without "-u"; > > - do all the merge-recursive computations in-core and prepare the >resulting index, while keeping the current index intact; > > - compare the current in-core index and the resulting in-core >index, and notice the paths that need to be added, updated or >removed in the working tree, and ensure that there is no loss of >information when the change is reflected to the working tree, >e.g. the result wants to create a file where the working tree >currently has a directory with non-expendable contents in it, the >result wants to remove a file where the working tree file has >local modification, etc.; and then finally > > - carry out the working tree update to make it match what the >resulting in-core index says it should look like. I had another email I had been composing to try to argue for changing merge-recursive.c's design to the above, assuming I could get the time to work on it. Nice to see that I'm not crazy, and that I apparently don't need to do much convincing. :-)
Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)
On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote: > On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote: >> But this line of though might be distracting from your original point, >> which was that we have so much to keep in mind when doing tree >> operations (flags, D/F conflicts, now submodules too). I wonder how >> a sensible refactoring would look like to detangle all these aspects, >> but still keeping Git fast and not overengineered. > > I think given how complex a lot of these code paths are, that an > attempt to refactor it a bit to detangle some of the mess would be > well worth the time. I'd suspect it might make handling the more > complex task of actually resolving conflicts to be easier, so the > effort to clean up the code here should be worth it. I think changing from a 4-way merge to a 3-way merge would make things much better, as Junio outlined here: https://public-inbox.org/git/xmqqd147kpdm@gitster.mtv.corp.google.com/ I don't know of any way to detangle the other aspects, yet.
Re: Clone repository computer A to remote B doenst work
Hi Roberto, On Sat, 25 Nov 2017, Roberto Garcia wrote: > I'm trying clone in windows a git repository to other remote machine > (NAS Linux based). > I have installed git for windows but i didn't installed nothing in the > other remote machine (NAS Linux based). You need a Git on the remote side. Otherwise Git will not be able to clone from there. Ciao, Johannes
Re: [PATCH v4 4/4] worktree: make add dwim
On Sat, 2017-11-25 at 20:06 +, Thomas Gummerer wrote: > This part is getting done in 3/4, and is definitely going to work > without an additional flag, so this is (hopefully) soon going to work > just as you want :) Yay! Thanks!
Re: [PATCH] submodule--helper.c: i18n: add a missing space in message
On Sat, Nov 25, 2017 at 2:55 PM, Jean-Noel Avila wrote:
> The message spans over 2 lines but the C conconcatenation does not add
s/conconcatenation/concatenation/
> the needed space between the two lines.
>
> Signed-off-by: Jean-Noel Avila
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2086f0eb0..a5c4a8a69 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -623,7 +623,7 @@ static void status_submodule(const char *path, const
> struct object_id *ce_oid,
>
> if (refs_head_ref(get_submodule_ref_store(path),
> handle_submodule_head_ref, &oid))
> - die(_("could not resolve HEAD ref inside the"
> + die(_("could not resolve HEAD ref inside the "
> "submodule '%s'"), path);
>
> print_status(flags, '+', path, &oid, displaypath);
> --
> 2.15.0
Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875
On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov wrote: > Thanks for the review. I saw only reaction of the Jeff in > the original thread and though that it is ok otherwise. I'm > fixing the things you mentioned. The commentary (in which you talked about restoring the patch and squashing) seemed to imply that this had been posted somewhere before, but it wasn't marked as "v2" (or whatever attempt) and lacked a URL pointing at the previous attempt, so it was difficult to judge. > On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote: >>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char >>> **out) >> >> Wrong data type: s/size_t req_len/ssize_t req_len/ > > Passing negative value to the function makes no sense. I > could add explicit type cast to make it clear. It should be > safe as site_t's range is bigger, and overflown > CONTENT_LENGTH results in die() at parsing (I have a test > which verifies it) A concern with requesting size_t bytes is that, if it does read all bytes, that value can't necessarily be represented by the ssize_t returned from the function. Where would the cast be placed that you suggest? How do other git functions deal with this sort of situation?
Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875
On Sat, Nov 25, 2017 at 07:38:33PM -0500, Eric Sunshine wrote: > On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov wrote: >> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote: +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out) >>> >>> Wrong data type: s/size_t req_len/ssize_t req_len/ >> >> Passing negative value to the function makes no sense. I >> could add explicit type cast to make it clear. It should be >> safe as site_t's range is bigger, and overflown >> CONTENT_LENGTH results in die() at parsing (I have a test >> which verifies it) > > A concern with requesting size_t bytes is that, if it does read all > bytes, that value can't necessarily be represented by the ssize_t > returned from the function. Where would the cast be placed that you > suggest? How do other git functions deal with this sort of situation? Right... ok, let it be ssize_t
Re: [PATCH v3 21/33] merge-recursive: add get_directory_renames()
Hi Elijah,
On Tue, 21 Nov 2017, Elijah Newren wrote:
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 2f4f85314a..6a0a6d4366 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1384,6 +1384,132 @@ static struct diff_queue_struct *get_diffpairs(struct
> merge_options *o,
> return ret;
> }
>
> +static void get_renamed_dir_portion(const char *old_path, const char
> *new_path,
> + char **old_dir, char **new_dir)
> +{
> + char *end_of_old, *end_of_new;
> + int old_len, new_len;
> +
> + *old_dir = NULL;
> + *new_dir = NULL;
> +
> + /* For
> + *"a/b/c/d/foo.c" -> "a/b/something-else/d/foo.c"
> + * the "d/foo.c" part is the same, we just want to know that
> + *"a/b/c" was renamed to "a/b/something-else"
> + * so, for this example, this function returns "a/b/c" in
> + * *old_dir and "a/b/something-else" in *new_dir.
> + *
> + * Also, if the basename of the file changed, we don't care. We
> + * want to know which portion of the directory, if any, changed.
> + */
> + end_of_old = strrchr(old_path, '/');
> + end_of_new = strrchr(new_path, '/');
> +
> + if (end_of_old == NULL || end_of_new == NULL)
> + return;
> + while (*--end_of_new == *--end_of_old &&
> +end_of_old != old_path &&
> +end_of_new != new_path)
> + ; /* Do nothing; all in the while loop */
> + /*
> + * We've found the first non-matching character in the directory
> + * paths. That means the current directory we were comparing
> + * represents the rename. Move end_of_old and end_of_new back
> + * to the full directory name.
> + */
> + if (*end_of_old == '/')
> + end_of_old++;
> + if (*end_of_old != '/')
> + end_of_new++;
> + end_of_old = strchr(end_of_old, '/');
> + end_of_new = strchr(end_of_new, '/');
> +
> + /*
> + * It may have been the case that old_path and new_path were the same
> + * directory all along. Don't claim a rename if they're the same.
> + */
> + old_len = end_of_old - old_path;
> + new_len = end_of_new - new_path;
> +
> + if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
> + *old_dir = strndup(old_path, old_len);
> + *new_dir = strndup(new_path, new_len);
These two callers of strndup() are the only ones in Git's code base now.
It is also causing a compile error on Windows.
Any reason you did not use xstrndup() here?
Ciao,
Dscho
Re: [PATCH v3 21/33] merge-recursive: add get_directory_renames()
On Sat, Nov 25, 2017 at 4:52 PM, Johannes Schindelin
wrote:
> On Tue, 21 Nov 2017, Elijah Newren wrote:
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> + if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
>> + *old_dir = strndup(old_path, old_len);
>> + *new_dir = strndup(new_path, new_len);
>
> These two callers of strndup() are the only ones in Git's code base now.
> It is also causing a compile error on Windows.
>
> Any reason you did not use xstrndup() here?
>
> Ciao,
> Dscho
Nope, was just unaware. I'll go ahead and switch them over for my
next roll of the series. Sorry for the pain.
[PATCH v4 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov
---
Makefile | 1 +
t/helper/test-print-values.c | 10 ++
t/t5560-http-backend-noserver.sh | 30 ++
3 files changed, 41 insertions(+)
create mode 100644 t/helper/test-print-values.c
diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-online-cpus
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
TEST_PROGRAMS_NEED_X += test-prio-queue
TEST_PROGRAMS_NEED_X += test-read-cache
TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 00..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include
+#include
+
+int cmd_main(int argc, const char **argv)
+{
+ if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+ printf("%zu", (ssize_t)(-20));
+
+ return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
expect_aliased 1 //domain/data.txt
'
+# overrides existing definition for further cases
+run_backend() {
+ CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+ ( echo "$2" && cat /dev/zero ) |
+ QUERY_STRING="${1#*[?]}" \
+ PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+ git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+ config http.uploadpack true &&
+ GET info/refs?service=git-upload-pack "200 OK" &&
+ ! grep "fatal:.*" act.err &&
+ POST git-upload-pack "200 OK" &&
+ ! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+ NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values"
"(size_t)(-20)"` &&
+ env \
+ CONTENT_TYPE=application/x-git-upload-pack-request \
+ QUERY_STRING=/repo.git/git-upload-pack \
+ PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+ GIT_HTTP_EXPORT_ALL=TRUE \
+ REQUEST_METHOD=POST \
+ CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+ git http-backend /dev/null 2>err &&
+ grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
test_done
--
2.11.0.1122.gc3fec58.dirty
[PATCH v5 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
I seem to forgot to put the authorship lines, and also did something with replies, so sending another sequence. v5: * marked the authorship more correctly v4: * polished style * add tests Max Kirillov (2): http-backend: respect CONTENT_LENGTH as specified by rfc3875 t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases Makefile | 1 + config.c | 8 config.h | 1 + http-backend.c | 39 ++- t/helper/test-print-values.c | 10 ++ t/t5560-http-backend-noserver.sh | 30 ++ 6 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-print-values.c -- 2.11.0.1122.gc3fec58.dirty
[PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
Author: Florian Manschwetus
Date: Wed, 30 Mar 2016 09:08:56 +
http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.
Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.
Signed-off-by: Florian Manschwetus
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov
---
config.c | 8
config.h | 1 +
http-backend.c | 39 ++-
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long
val)
return val;
}
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+ const char *v = getenv(k);
+ if (v && !git_parse_ssize_t(v, &val))
+ die("failed to parse %s", k);
+ return val;
+}
+
int git_config_system(void)
{
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *,
const char *, const c
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 ssize_t git_env_ssize_t(const char *, ssize_t);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
#if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..b4ba83b624 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf
*hdr, const char *name)
* hit max_request_buffer we die (we'd rather reject a
* maliciously large request than chew up infinite memory).
*/
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
{
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)
}
}
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char
**out)
+{
+ unsigned char *buf = NULL;
+ ssize_t cnt = 0;
+
+ if (max_request_buffer < req_len) {
+ die("request was larger than our maximum size (%lu): %lu;"
+ " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+ max_request_buffer,
+ req_len);
+ }
+
+ if (req_len <= 0) {
+ *out = NULL;
+ return 0;
+ }
+
+ buf = xmalloc(req_len);
+ cnt = read_in_full(fd, buf, req_len);
+ if (cnt < 0) {
+ free(buf);
+ return -1;
+ } else {
+ *out = buf;
+ return cnt;
+ }
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+ ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+ if (req_len < 0)
+ return read_request_eof(fd, out);
+ else
+ return read_request_fixed_len(fd, req_len, out);
+}
+
static void inflate_request(const char *prog_name, int out, int buffer_input)
{
git_zstream stream;
--
2.11.0.1122.gc3fec58.dirty
[PATCH v4 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.
Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.
Signed-off-by: Florian Manschwetus
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov
---
config.c | 8
config.h | 1 +
http-backend.c | 39 ++-
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long
val)
return val;
}
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+ const char *v = getenv(k);
+ if (v && !git_parse_ssize_t(v, &val))
+ die("failed to parse %s", k);
+ return val;
+}
+
int git_config_system(void)
{
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *,
const char *, const c
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 ssize_t git_env_ssize_t(const char *, ssize_t);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
#if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..b4ba83b624 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf
*hdr, const char *name)
* hit max_request_buffer we die (we'd rather reject a
* maliciously large request than chew up infinite memory).
*/
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
{
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)
}
}
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char
**out)
+{
+ unsigned char *buf = NULL;
+ ssize_t cnt = 0;
+
+ if (max_request_buffer < req_len) {
+ die("request was larger than our maximum size (%lu): %lu;"
+ " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+ max_request_buffer,
+ req_len);
+ }
+
+ if (req_len <= 0) {
+ *out = NULL;
+ return 0;
+ }
+
+ buf = xmalloc(req_len);
+ cnt = read_in_full(fd, buf, req_len);
+ if (cnt < 0) {
+ free(buf);
+ return -1;
+ } else {
+ *out = buf;
+ return cnt;
+ }
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+ ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+ if (req_len < 0)
+ return read_request_eof(fd, out);
+ else
+ return read_request_fixed_len(fd, req_len, out);
+}
+
static void inflate_request(const char *prog_name, int out, int buffer_input)
{
git_zstream stream;
--
2.11.0.1122.gc3fec58.dirty
[PATCH v5 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov
---
Makefile | 1 +
t/helper/test-print-values.c | 10 ++
t/t5560-http-backend-noserver.sh | 30 ++
3 files changed, 41 insertions(+)
create mode 100644 t/helper/test-print-values.c
diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-online-cpus
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
TEST_PROGRAMS_NEED_X += test-prio-queue
TEST_PROGRAMS_NEED_X += test-read-cache
TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 00..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include
+#include
+
+int cmd_main(int argc, const char **argv)
+{
+ if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+ printf("%zu", (ssize_t)(-20));
+
+ return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
expect_aliased 1 //domain/data.txt
'
+# overrides existing definition for further cases
+run_backend() {
+ CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+ ( echo "$2" && cat /dev/zero ) |
+ QUERY_STRING="${1#*[?]}" \
+ PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+ git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+ config http.uploadpack true &&
+ GET info/refs?service=git-upload-pack "200 OK" &&
+ ! grep "fatal:.*" act.err &&
+ POST git-upload-pack "200 OK" &&
+ ! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+ NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values"
"(size_t)(-20)"` &&
+ env \
+ CONTENT_TYPE=application/x-git-upload-pack-request \
+ QUERY_STRING=/repo.git/git-upload-pack \
+ PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+ GIT_HTTP_EXPORT_ALL=TRUE \
+ REQUEST_METHOD=POST \
+ CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+ git http-backend /dev/null 2>err &&
+ grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
test_done
--
2.11.0.1122.gc3fec58.dirty
[PATCH v4 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
* polished style * add tests * marked the authorship more correctly Max Kirillov (2): http-backend: respect CONTENT_LENGTH as specified by rfc3875 t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases Makefile | 1 + config.c | 8 config.h | 1 + http-backend.c | 39 ++- t/helper/test-print-values.c | 10 ++ t/t5560-http-backend-noserver.sh | 30 ++ 6 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-print-values.c -- 2.11.0.1122.gc3fec58.dirty
Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875
On Fri, Nov 24, 2017 at 02:54:50PM +0900, Junio C Hamano wrote: > Max Kirillov writes: >> I hope I marked it correctly in the trailers. > > It is probably more conventional to do it like so: > > From: Florian Manschwetus > Date: > > Signed-off-by: Florian Manschwetus > [mk: fixed trivial build failures and stuff] > Signed-off-by: Max Kirillov Thanks. Have done so
Bug in pathspec handling (in conjunction with submodules)
Hi Duy & Brandon, in 74ed43711fd (grep: enable recurse-submodules to work on objects, 2016-12-16), the do_match() function in tree-walk.c was changed so that it can recurse across submodule boundaries. However, there is a bug, and I *think* there may be two bugs actually. Or even three. First of all, here is an MCVE that I distilled from https://github.com/git-for-windows/git/issues/1371: git init repo cd repo git init submodule git -C submodule commit -m initial --allow-empty touch "[bracket]" git add "[bracket]" git commit -m bracket git add submodule git commit -m submodule git rev-list HEAD -- "[bracket]" Nothing fancy, just adding a file with brackets in the name, then a submodule, then showing the commit history filtered by the funny file name. However, the log prints *both* commits. Clearly the submodule commit should *not* be shown. Now, how does this all happen? Since the pathspec contains brackets, parse_pathspec() marks it as containing wildcards and sets nowildcard_len to 0. Now, note that [bracket] *is* a wildcard expression: it should only match a single character that is one of a, b, c, e, k, r or t. I think this is the first bug: `git rev-list` should not even match the commit that adds the file [bracket] because its file name does not match that expression. From where I sit, it would appear that f1a2ddbbc2d (tree_entry_interesting(): optimize wildcard matching when base is matched, 2010-12-15) simply added the fnmatch() code without disabling the literal match_entry() code when the pathspec contains a pattern. But it does not stop there: there is *another* bug which causes the pattern to somehow match the submodule. I *guess* the idea of https://github.com/git/git/commit/74ed43711#diff-7a08243175f2cae66aedf53f7dce3bdfR1015 was to allow a pattern like *.c to match files in a submodule, but the pattern [bracket] should not match any file in submodule/. I think that that code needs to be a little bit more careful to try to match the submodule's name against the pattern (it seems to interpret nowildcard_len == 0 to mean that the wildcard is `*`). However, the commit introducing that code wanted to teach *grep* (not *rev-list*) a new trick, and it relies on the `recursive` flag of the pathspec to be set. And now it gets really interesting. Or confusing, depending on your mental condition. This recursive flag of the pathspec is set in ll_diff_tree_paths() (yep, changing the flag in the passed-in opt structure... which I found a bit... unexpected, given the function name, I would have been less surprised if that function only diff'ed the trees and used the options without changing the options). That flag-change was introduced in https://github.com/git/git/commit/bc96cc87dbb2#diff-15203e8cd8ee9191113894de9d97a8a6R149 which is another patch that changed the tree diff machinery to accommodate `git grep` (but maybe not really paying a lot of attention to the fact that the same machinery is called repeatedly by the revision machinery, too). I am really confused by this code mainly due to the fact that the term "recursive" is pretty ambiguous in that context: does it refer to directories/tree objects, or to submodules? I guess it is used for both when there should be two flags so that rev-list can recurse over tree objects but not submodules (unless told to do so). The problem, of course, is that `git rev-list HEAD -- '[bracket]'` never recurses into the submodule. And therefore, the promised "more accurate matching [...] in the submodule" is never performed. And the commit adding the submodule is never pruned. Since I am not really familiar with all that tree diff code (and as a general rule to protect my mental health, I try my best to stay away from submodules, too), but you two are, may I ask you gentle people to have a closer look to fix those bugs? Thanks, Dscho
Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals
Elijah Newren writes: > I had another email I had been composing to try to argue for changing > merge-recursive.c's design to the above, assuming I could get the time > to work on it. Nice to see that I'm not crazy, and that I apparently > don't need to do much convincing. :-) You might even be better off coming up with a *new* merge strategy backend if you want to do this, without using much from the existing code in merge-recursive.c at all (I've written off that code as mostly unsalvageable long time ago, and thanked for whoever had a clever idea to allow different strategy backend to be made without disrupting the rest of the system). After we gain more confidence with the rewrite, we can switch the internally built-in backend used by different codepaths from merge-recursive.c::merge_recursive() to the new thing.
[PATCH] pretty: fix buffer over-read with %> and %
A buffer over-read of the format string would occur with unterminated formats of the form '%>(#' and '%<(#', where '#' represents a number. This error can be witnessed by running git log under valgrind like so: valgrind git log -n1 --format='%<(42' This was due to the fact that the "not found" case for strcspn() was being checked in the same way that one checks the "not found" case for strchr(), i.e. by checking for a NULL pointer return value. Instead, one must check for the end of the string since strcspn() points to the character where it finished its search (which will be a '\0' if unsuccessful). Signed-off-by: mwnx --- pretty.c | 2 +- t/t4205-log-pretty-formats.sh | 6 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index 2f6b0ae6c..4c70bad45 100644 --- a/pretty.c +++ b/pretty.c @@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb, const char *end = start + strcspn(start, ",)"); char *next; int width; - if (!end || end == start) + if (!*end || end == start) return 0; width = strtol(start, &next, 10); if (next == start || width == 0) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 591f35daa..4d9555962 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'unterminated alignment formatting' ' + git log -n1 --format="%<(42" >actual && + echo "%<(42" >expected && + test_cmp expected actual +' + test_done -- 2.15.0.90.g6559daec7
Re: [PATCH v4 1/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value
Junio C Hamano writes:
> Thanks for sticking with this topic---very much appreciated, as we
> saw many newcomers get tired of doing repeated polishing and abandon
> their topic in the past. I have to go offline now, but will comment
> later when I come back.
Regarding the overall structure of the series, I think 1/6 will
break tests and the breakage will stay until it is fixed at the
last step. Also, now print_sha1_ellipsis() is prominently not about
"diff", it may want to move out of "diff.[ch]".
So, perhaps this may be a better structure:
- 1/6: What we see in [v4 4/6] as an independent typofix.
- 2/6: What we see in [v4 3/6] that is not about the output from
"git checkout" (e.g. the use of ellipsis to shorten the
example input the user gives, as if the user is using full
40-hex and we are not bothering to show everything in the
documentation), with "there is no need to use full 40-hex to
identify the object names like the examples hint at by
omitting the tail part of an object name as if that has to
be spelled out but the example omits them only for
brevity. Give examples using an abbreviated object names
without ellipses just like how people do in real life", or
something like that, as an explanation.
- 3/6: Introduce a helper print_sha1_ellipsis() that pays attention
to GIT_PRINT_SHA1_ELLIPSIS environment, and prepares the
tests to unconditionally set it for the test pieces that
will be broken once the code stops showing the extra dots by
default. What we see in [v4 5/6] may also want to be rolled
into this patch.
Nobody calls print_sha1_ellipsis() function yet at this
step. cache.h and environment.c may be a better place for
the declaration and definition of the helper.
Mention that the removal of these dots is merely a plan at
this step and has not happened yet but soon will be in the
proposed log message.
- 4/6: What we see in [v4 2/6], optionally with two additional
tests that looks at the output with and without the
environment variable (it is optional because the output is
purely for human consumption sent to the standard error
stream). Parts of [v4 3/6] about the output from the "git
checkout" command also belongs to this step.
- 5/6: What we see in [v4 1/6] (except for introduction of the
helper, which already happened in an earlier step). Move
the promise of covering this new output format with a follow
up series to the proposed log message of this step from [v4
6/6].
- 6/6: Realize the promise made in 5/6.
Alternatively, just like the step 3/6 above prepares the
tests for "upcoming" feature without exercising that
preparation, a part of this can be split to allow annotating
the "here-document" test vectors with "does this test expect
that ellipsis will be missing in the output?" (i.e. change
the logic in the loop that parses "<8 --
Subject: [PATCH] t4013: prepare for upcoming "diff --raw --abbrev" format change
Most of the t4013 tests go through a list of sample command lines,
and each of them is executed and its output compared with an
expected one stored in t4013/ directory. Allow these lines to begin
with a colon followed by magic word(s) so that test conditions can
easily be tweaked.
The expected use that will happen in later steps of this is to run
tests expecting the traditional output and run the same test without
the GIT_PRINT_SHA1_ELLIPSIS=yes environment exported for (perhaps
some of) them, which will have to expect different output. Since
all of the existing tests are meant to run with the environment,
use the magic word "noellipses" to cause the variable not to be set
and exported.
As this step does not add any new test with the magic word, all
tests still run with the environment variable, expecting the
traditional output, but it will change soon.
Signed-off-by: Junio C Hamano
---
t/t4013-diff-various.sh | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 9bed64d53e..7288b54045 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -118,20 +118,37 @@ test_expect_success setup '
EOF
V=$(git version | sed -e 's/^git version //' -e 's/\./\\./g')
-while read cmd
+while read magic cmd
do
- case "$cmd" in
- '' | '#'*) continue ;;
+ case "$magic" in
+ '' | '#'*)
+ continue ;;
+ :*)
+ magic=${magic#:}
+ label="$magic-$cmd"
+ case "$magic" in
+ noellipses) ;;
+ *)
+ die "bug in t4103: unknown magic $magic" ;;
+ esac ;;
+ *)
+ cmd="$magic $cmd" magic=
+ label="$cmd" ;;
Re: [PATCH v4 1/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value
... and this is a sample of the "remainder" of 6/6 outlined in the previous message, to become part of 5/6 to show how the new output would look like in a test form. t/t4013-diff-various.sh| 1 + t/t4013/diff.noellipses-diff-tree_--root_-r_--abbrev=4_initial | 6 ++ 2 files changed, 7 insertions(+) create mode 100644 t/t4013/diff.noellipses-diff-tree_--root_-r_--abbrev=4_initial diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 7288b54045..8b5474a087 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -178,6 +178,7 @@ diff-tree --root --abbrev initial diff-tree --root -r initial diff-tree --root -r --abbrev initial diff-tree --root -r --abbrev=4 initial +:noellipses diff-tree --root -r --abbrev=4 initial diff-tree -p initial diff-tree --root -p initial diff-tree --patch-with-stat initial diff --git a/t/t4013/diff.noellipses-diff-tree_--root_-r_--abbrev=4_initial b/t/t4013/diff.noellipses-diff-tree_--root_-r_--abbrev=4_initial new file mode 100644 index 00..26fbfeb177 --- /dev/null +++ b/t/t4013/diff.noellipses-diff-tree_--root_-r_--abbrev=4_initial @@ -0,0 +1,6 @@ +$ git diff-tree --root -r --abbrev=4 initial +444ac553ac7612cc88969031b02b3767fb8a353a +:00 100644 35d2 A dir/sub +:00 100644 01e7 A file0 +:00 100644 01e7 A file2 +$ -- 2.15.0-479-ge11ee266c9
Re: [PATCH v4 1/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value
Junio C Hamano writes: > - 5/6: What we see in [v4 1/6] (except for introduction of the > helper, which already happened in an earlier step). Move > the promise of covering this new output format with a follow > up series to the proposed log message of this step from [v4 > 6/6]. > > - 6/6: Realize the promise made in 5/6. > ... > For rererence, here is what I just whipped up that would come in > between the steps 4/6 and 5/6 in the above outline (i.e. split from > 6/6 to be placed before 5/6 happens). Just to be sure, I wrote this on top of [v4 1-6/6], so you'd see that its preimage already knows about GIT_PRINT_SHA1_ELLIPSIS=yes. In a real reroll of the series that follow the above reorganized structure, of course that would not appear as "-" line in the patch ;-) Thanks.
Re: git status always modifies index?
Jeff King writes: > What I was trying to get at is that naming it "status --no-lock-index" > would not be the same thing (even though with the current implementation > it would behave the same). IOW, can we improve the documentation of > "status" to point to make it easier to discover this use case. Yeah, the name is unfortunate. What the end user really wants to see, I suspect, is a "--read-only" option that applies to any filesystem entity and to any command, in the context of this thread, and also in the original discussion that led to the introduction of that option. While I think the variable losing "index" from its name was a vast improvement relative to "--no-lock-index", simply because it expresses what we do a bit closer to "do not just do things without modifying anything my repository", it did not go far enough.
Re: [PATCH v4 4/4] worktree: make add dwim
Thomas Gummerer writes: > Of course that assumes that it's used directly, not in scripts, and > that users will actually read the output of the command when they > invoke it. Maybe these are not safe assumptions to make though, and > we'd rather not have this on by default then. None of these is a safe assumption to make. With a transition plan, we could change the default eventually, but if this feature is already used by real users, the proposed change without any transition plan will hurt them and make them hate us, I am afraid.
Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
Max Kirillov writes:
> Author: Florian Manschwetus
This should read "From: ...";
> Date: Wed, 30 Mar 2016 09:08:56 +
>
> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. Web server may exercise the specification by not closing
> the script's standard input after writing content. In that case http-backend
> would hang waiting for the input. The issue is known to happen with
> IIS/Windows, for example.
>
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus
> [mk: fixed trivial build failures and polished style issues]
>
> Signed-off-by: Max Kirillov
There shouldn't be a blank line before your sign-off.
The above are only for future reference; I can fix them up before
queuing if there isn't any other issues in this patch.
> +ssize_t git_env_ssize_t(const char *k, ssize_t val)
> +{
> + const char *v = getenv(k);
> + if (v && !git_parse_ssize_t(v, &val))
> + die("failed to parse %s", k);
> + return val;
> +}
> +
If this were passing "v" that is a string the caller obtains from
any source (and the callee does not care about where it came from),
that would be a different story, but as a public interface that is
part of the config.c layer, "k" that has to be the name of the
environment variable sticks out like a sore thumb.
If we were to add one more public function to the interface, I
suspect that exposing the existing git_parse_ssize_t() and have the
caller implement the above function for its use would be a much
better way to go.
Re: [PATCH] RelNotes: minor typo fixes in 2.15.1 draft
Todd Zullinger writes: > It was mildly surprising that the script didn't warn or complain about > an unknown option. After a quick look, that seems to be due to the > Getopt::Long pass_through option which sends unknown options to > format-patch. Heh, that is another reason why I recommend against letting send-email run format-patch from within (another a lot bigger one is that running them seprately gives you a chance to look and proofread the output from format-patch one more time before actually sending them out, which also allows you to update additional comments after the three-dash lines).
Re: [PATCH] submodule--helper.c: i18n: add a missing space in message
Thanks.
Re: [PATCH] Makefile: check that tcl/tk is installed
Christian Couder writes: > On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder > wrote: >> By default running `make install` in the root directory of the >> project will set TCLTK_PATH to `wish` and then go into the "git-gui" >> and "gitk-git" sub-directories to build and install these 2 >> sub-projects. > > Has this patch fallen through the cracks or is there an unresolved issue? I had an impression that the conclusion was that the existing error message at runtime already does an adequate job and there is no issue to be addressed by this patch. Am I mistaken?
Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)
On Sat, Nov 25, 2017 at 2:37 PM, Elijah Newren wrote: > On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote: >> On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote: > >>> But this line of though might be distracting from your original point, >>> which was that we have so much to keep in mind when doing tree >>> operations (flags, D/F conflicts, now submodules too). I wonder how >>> a sensible refactoring would look like to detangle all these aspects, >>> but still keeping Git fast and not overengineered. >> >> I think given how complex a lot of these code paths are, that an >> attempt to refactor it a bit to detangle some of the mess would be >> well worth the time. I'd suspect it might make handling the more >> complex task of actually resolving conflicts to be easier, so the >> effort to clean up the code here should be worth it. > > I think changing from a 4-way merge to a 3-way merge would make things > much better, as Junio outlined here: > > https://public-inbox.org/git/xmqqd147kpdm@gitster.mtv.corp.google.com/ > > I don't know of any way to detangle the other aspects, yet. I agree, that is absolutely a (big) step in the right direction. Thanks, Jake
Problem with environment of hook execution when git is run with --work-tree / --git-dir
Hi! I noticed a potential bug with the invocation of a pre-commit hook when running git with --work-tree and --git-dir. In particular, I was investigating how hooks can still run git commands properly even when the work-tree or git-dir is overridden via CLI args. I put the following in "/home/mgsloan/.dotfiles-git/hooks/pre-commit": #!/bin/sh env after this, running "git --work-tree=/home/mgsloan --git-dir=/home/mgsloan/.dotfiles-git commit" has output with a bunch of variables, here are the important ones: GIT_WORK_TREE=. GIT_DIR=.dotfiles-git/ PWD=/home/mgsloan So what's the problem with this choice of environment variables? Well, the problem is that if PWD is changed during the execution of the script, then GIT_WORK_TREE and GIT_DIR will no longer work properly. For example, if the pre-commit hook is #!/bin/sh cd some-dir git status This will fail with Not a git repository: '.dotfiles-git' There is another detail here, which is that when --git-dir / --work-tree is not specified, the no GIT_WORK_TREE / GIT_DIR environment variable is set. This means that in this case, changing PWD in the hook will work fine as long as the search for .git will find the right one. Note that this also means that changing PWD in a script can change which git repo the command is being run on, for example, when the hook is interacting with a submodule. A half-fix to this would be to have the GIT_WORK_TREE and GIT_DIR set when running hooks use absolute paths. However, this would not have the same behavior as when git is used without --git-dir / --work-tree. As described in the paragraph above, if PWD is relied upon to instead target a different git repo, then things break. Not sure what the total fix for this would be. I think the information that needs to be conveyed to the hook's git invocations is that "the work-tree /home/mgsloan should be associated with the git-dir /home/mgsloan/.dotfiles-git". Could have an env var like GIT_DIR_MAPPINGS="/home/mgsloan!/home/mgsloan/.dotfiles-git" The idea is that this would be a list of mappings from GIT_WORK_TREE to GIT_DIR. If this variable is set, then it will be followed when git is searching parents of PWD for ".git" directories. I chose "!" rather arbitrarily here. "->" would look nicer, but people might forget to escape it when programmatically setting this var. What do y'all think of this idea? Some of you might be wondering what I'm doing with my work tree being my home directory. It is the approach suggested here - https://developer.atlassian.com/blog/2016/02/best-way-to-store-dotfiles-git-bare-repo/ - for versioning your configuration files directly. Apologies if this has already been discussed, I could not find a good way to search the mailinglist archives. Thanks! -Michael
