Re: [PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
On 26-Oct-18 3:13 AM, Junio C Hamano wrote: Slavica Djukic writes: From: Slavica Please make sure this matches your sign-off below. This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. As the four lines above summarize the issue being highlighted by the expect-failure rather well, the last two lines are unnecessary. Please remove them. Alternatively, you can place them after the three-dash lines we see below. Signed-off-by: Slavica Djukic --- t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9e06494ba0..ae2c905343 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash works when user.name and user.email are not set' ' +test_commit 1 && Just being curious, but do we need a fresh commit created at this point in the test? Many tests before this one begin with "git reset" and then run "git stash" without ever creating commit themselves, instead relying on the fact that there already is at least one commit created in the "setup" phase of the test that a "stash" created can be made relative to. I do not think this test is all that special in that regard to require its own commit. No, we don't need fresh commit here. Thank you for this and all other suggestions. I've changed test according to them. +test_config user.useconfigonly true && +test_config stash.usebuiltin true && +sane_unset GIT_AUTHOR_NAME && +sane_unset GIT_AUTHOR_EMAIL && +sane_unset GIT_COMMITTER_NAME && +sane_unset GIT_COMMITTER_EMAIL && +test_unconfig user.email && There are trailing whitespaces on the line above. Please remove. Also, Don't be original in the form alone---all other tests in this file indent with a leading HT, not four SPs. Please match the style of surrounding code. +test_unconfig user.name && +echo changed >1.t && +git stash +' + test_done Thanks. Please do not reroll the next round at too rapid a pace. I've taken my time for next round, I am working on 2/3 and 3/3 parts as well. I wouldn't have sent this patch if I understood you well in previous reply. Thank you, Slavica.
Re: [PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica Djukic writes: > From: Slavica Please make sure this matches your sign-off below. > This is part of enhancement request that ask for 'git stash' to work > even if 'user.name' and 'user.email' are not configured. > Due to an implementation detail, git-stash undesirably requires > 'user.name' and 'user.email' to be set, but shouldn't. > The issue is discussed here: > https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. As the four lines above summarize the issue being highlighted by the expect-failure rather well, the last two lines are unnecessary. Please remove them. Alternatively, you can place them after the three-dash lines we see below. > Signed-off-by: Slavica Djukic > --- > t/t3903-stash.sh | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 9e06494ba0..ae2c905343 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with > binary files' ' > test_path_is_file subdir/untracked > ' > > +test_expect_failure 'stash works when user.name and user.email are not set' ' > +test_commit 1 && Just being curious, but do we need a fresh commit created at this point in the test? Many tests before this one begin with "git reset" and then run "git stash" without ever creating commit themselves, instead relying on the fact that there already is at least one commit created in the "setup" phase of the test that a "stash" created can be made relative to. I do not think this test is all that special in that regard to require its own commit. > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +sane_unset GIT_AUTHOR_NAME && > +sane_unset GIT_AUTHOR_EMAIL && > +sane_unset GIT_COMMITTER_NAME && > +sane_unset GIT_COMMITTER_EMAIL && > +test_unconfig user.email && There are trailing whitespaces on the line above. Please remove. Also, Don't be original in the form alone---all other tests in this file indent with a leading HT, not four SPs. Please match the style of surrounding code. > +test_unconfig user.name && > +echo changed >1.t && > +git stash > +' > + > test_done Thanks. Please do not reroll the next round at too rapid a pace.
[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
From: Slavica This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. Signed-off-by: Slavica Djukic --- t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9e06494ba0..ae2c905343 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash works when user.name and user.email are not set' ' +test_commit 1 && +test_config user.useconfigonly true && +test_config stash.usebuiltin true && +sane_unset GIT_AUTHOR_NAME && +sane_unset GIT_AUTHOR_EMAIL && +sane_unset GIT_COMMITTER_NAME && +sane_unset GIT_COMMITTER_EMAIL && +test_unconfig user.email && +test_unconfig user.name && +echo changed >1.t && +git stash +' + test_done -- 2.19.1.windows.1
[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name
Changes since v1: *changed: test_must_fail git config user.email to: test_unconfig user.email && test_unconfig user.name This is done to make sure that user.email and user.name are not set, instead of asserting it with test_must_fail config user.email. Slavica (1): [Outreachy] t3903-stash: test without configured user name t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) -- 2.19.1.windows.1
Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
Eric Sunshine writes: >> +test_commit 1 && >> +test_config user.useconfigonly true && >> +test_config stash.usebuiltin true && >> +sane_unset GIT_AUTHOR_NAME && >> +sane_unset GIT_AUTHOR_EMAIL && >> +sane_unset GIT_COMMITTER_NAME && >> +sane_unset GIT_COMMITTER_EMAIL && >> +test_must_fail git config user.email && > > Instead of simply asserting that 'user.email' is not set here, you > could instead proactively ensure that it is not set. That is, instead > of the test_must_fail(), do this: > > test_unconfig user.email && > test_unconfig user.name && Yes, it would be more in line with what is done to the environment variables and to other configuration variables in the same block. Not that I think that this inconsistency is end of the world ;-) Thanks. >> +echo changed >1.t && >> +git stash >> +' >> + >> test_done >> -- >> 2.19.1.windows.1 >>
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica writes: > On 23-Oct-18 8:52 PM, Christian Couder wrote: >> On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote: >>> This is part of enhancement request that ask for `git stash` to work even >>> if `user.name` is not configured. >>> The issue is discussed here: >>> https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. >> We prefer commit messages that contain as much as possible all the >> information necessary to understand the patch without links to other >> places. >> >> It seems that only this email from you reached me. Did you send other >> emails for patches 2/3 and 3/3? >> >> [...] > > Okay, I will change that. This is my first patch and I am still adapting. > > Emails for patches 2/3 and 3/3 because aren't there because I am still > preparing them. > > (I didn't know if I had 3 patches in plan that they should be sent at > almost the same time.) It is more efficient for everybody involved. - You may discover that 1/3 you just (thought) finished was not sufficient while working on 2/3 and 3/3, and by the time you are pretty close to finishing 2/3 and 3/3, you may want to update 1/3 in a big way. Sending a premature version and having others to review is wasting everbody's time. - Your 1/3 might become perfect alone with help from others' reviews and your updates, but after that everybody may forget about it when you are ready to send out 2/3 and 3/3; if these three are truly related patches in a single topic, you would want to have what 1/3 did fresh in your reviewers' minds. You'd have to find the old message of 1/3 and make 2/3 and 3/3 responses to it to keep them properly threaded (which may take your time), and reviewers need to refresh their memory by going back to 1/3 before reviewing 2/3 and 3/3 One thing I learned twice while working in this project is that open source development is not a race to produce and show your product as quickly as possible. When I was an individual contributor, the project was young and there were many people with good and competing ideas working to achieve more-or-less the same goal. It felt like a competition to get *MY* version of the vision, design and implementation over others' adopted and one way to stay in the competition was to send things as quickly as possible. I didn't know better, and I think I ended up wasting many people's time that way. That changed when I became the maintainer, as (1) I no longer had to race with anybody ;-), and (2) I introduced the 'pu' (proposed update) system so that anything that was queued early can be discarded and replaced when a better thing come within a reasonable timeframe. And then I re-learned the same "this is not a race" lesson a couple of years ago, when I started working in a timezone several hours away from the most active participants for a few months at a time. I do not have to respond to a message I see on the list immediately, as it is too late to catch the sender who is already in bed ;-) So take your time and make sure what you are sending out can be reviewed the most efficiently. Completing 2/3 and 3/3 before sending 1/3 out to avoid having to redo 1/3 and avoid having reviewers to spend their time piecemeal is one thing. Making sure that the patch does not have style issues that distract reviewers' attention is another. Sitting on what you think you have completed for a few days allows you to review your product with fresh eyes before sending them out, which is another benefit of trying not to rush.
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Johannes Schindelin writes: >> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do >> so to avoid getting affected by the real $HOME/.gitconfig of the >> user who happens to be running the test suite. > > My bad. I should have checked. I was under the impression that we set > `HOME` to the `t/` directory and initialized it. But you are right, of > course, and that subshell as well as the override of `HOME` are absolutely > unnecessary. I was afraid that I may be missing some future plans to update $TRASH_DIRECTORY/.gitconfig with "git config --global user.name Foo" etc. in an earlier part of the test script, which would have made the subshell and moving HOME elsewhere perfectly good ways to future proof the new test being added (in which case, in-code comment to say that near the assignment to HOME would have been a good improvement). Not that having them breaks the logic, but they distract the readers by making them wonder what is going on, so I think we can do without the subshell and assignment to HOME. Thanks.
Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
On Wed, Oct 24, 2018 at 4:06 PM Slavica Djukic wrote: > This is part of enhancement request that ask for 'git stash' to work > even if 'user.name' and 'user.email' are not configured. > Due to an implementation detail, git-stash undesirably requires > 'user.name' and 'user.email' to be set, but shouldn't. Thanks for re-rolling. This version looks better. One comment below... > Signed-off-by: Slavica Djukic > --- > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > @@ -1156,4 +1156,17 @@ test_expect_success 'stash -- works with > binary files' ' > +test_expect_failure 'stash works when user.name and user.email are not set' ' > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +sane_unset GIT_AUTHOR_NAME && > +sane_unset GIT_AUTHOR_EMAIL && > +sane_unset GIT_COMMITTER_NAME && > +sane_unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && Instead of simply asserting that 'user.email' is not set here, you could instead proactively ensure that it is not set. That is, instead of the test_must_fail(), do this: test_unconfig user.email && test_unconfig user.name && > +echo changed >1.t && > +git stash > +' > + > test_done > -- > 2.19.1.windows.1 >
[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
From: Slavica This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. Signed-off-by: Slavica Djukic --- t/t3903-stash.sh | 13 + 1 file changed, 13 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9e06494ba0..048998d5ce 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1156,4 +1156,17 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash works when user.name and user.email are not set' ' +test_commit 1 && +test_config user.useconfigonly true && +test_config stash.usebuiltin true && +sane_unset GIT_AUTHOR_NAME && +sane_unset GIT_AUTHOR_EMAIL && +sane_unset GIT_COMMITTER_NAME && +sane_unset GIT_COMMITTER_EMAIL && +test_must_fail git config user.email && +echo changed >1.t && +git stash +' + test_done -- 2.19.1.windows.1
[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
Changes since v1: *changed test title *removed subshell and HOME override *fixed weird identation *unset() replaced with sane_unset() Slavica (1): [Outreachy] t3903-stash: test without configured user name t/t3903-stash.sh | 13 + 1 file changed, 13 insertions(+) -- 2.19.1.windows.1
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Hi Junio, On Wed, 24 Oct 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Wed, 24 Oct 2018, Junio C Hamano wrote: > > > >> Slavica writes: > >> > >> > +test_expect_failure 'stash with HOME as non-existing directory' ' > >> > +test_commit 1 && > >> > +test_config user.useconfigonly true && > >> > +test_config stash.usebuiltin true && > >> > +( > >> > +HOME=$(pwd)/none && > >> > +export HOME && > >> > >> What is the reason why this test needs to move HOME away from > >> TRASH_DIRECTORY (set in t/test-lib.sh)? > > > > This is to make sure that no user.name nor user.email is configured. That > > was my idea, hence I answer your question. > > HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do > so to avoid getting affected by the real $HOME/.gitconfig of the > user who happens to be running the test suite. My bad. I should have checked. I was under the impression that we set `HOME` to the `t/` directory and initialized it. But you are right, of course, and that subshell as well as the override of `HOME` are absolutely unnecessary. Thanks, Dscho > > With that in place for ages, this test still moves HOME away from > TRASH_DIRECTORY, and that is totally unnecessary if it is only done > to avoid getting affected by the real $HOME/.gitconfig of the user. > After all, this single test is not unique in its need to avoid > reading from user's $HOME/.gitconfig---so I expected there may be > other reasons. > > That is why I asked, and your response is not quite answering that > question. >
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
On 23-Oct-18 8:52 PM, Christian Couder wrote: On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote: This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. We prefer commit messages that contain as much as possible all the information necessary to understand the patch without links to other places. It seems that only this email from you reached me. Did you send other emails for patches 2/3 and 3/3? [...] Okay, I will change that. This is my first patch and I am still adapting. Emails for patches 2/3 and 3/3 because aren't there because I am still preparing them. (I didn't know if I had 3 patches in plan that they should be sent at almost the same time.) +( +HOME=$(pwd)/none && +export HOME && +unset GIT_AUTHOR_NAME && +unset GIT_AUTHOR_EMAIL && +unset GIT_COMMITTER_NAME && +unset GIT_COMMITTER_EMAIL && +test_must_fail git config user.email && +echo changed >1.t && + git stash It seems that the above line is not indented like the previous ones. I don't know what is the reason, in my IDE everything seems fine, but I'll fix it. +) +' Thanks for contributing, Christian. You are welcome, Slavica
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Johannes Schindelin writes: > Hi Junio, > > On Wed, 24 Oct 2018, Junio C Hamano wrote: > >> Slavica writes: >> >> > +test_expect_failure 'stash with HOME as non-existing directory' ' >> > +test_commit 1 && >> > +test_config user.useconfigonly true && >> > +test_config stash.usebuiltin true && >> > +( >> > +HOME=$(pwd)/none && >> > +export HOME && >> >> What is the reason why this test needs to move HOME away from >> TRASH_DIRECTORY (set in t/test-lib.sh)? > > This is to make sure that no user.name nor user.email is configured. That > was my idea, hence I answer your question. HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do so to avoid getting affected by the real $HOME/.gitconfig of the user who happens to be running the test suite. With that in place for ages, this test still moves HOME away from TRASH_DIRECTORY, and that is totally unnecessary if it is only done to avoid getting affected by the real $HOME/.gitconfig of the user. After all, this single test is not unique in its need to avoid reading from user's $HOME/.gitconfig---so I expected there may be other reasons. That is why I asked, and your response is not quite answering that question.
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Hi Junio, On Wed, 24 Oct 2018, Junio C Hamano wrote: > Slavica writes: > > > +test_expect_failure 'stash with HOME as non-existing directory' ' > > +test_commit 1 && > > +test_config user.useconfigonly true && > > +test_config stash.usebuiltin true && > > +( > > +HOME=$(pwd)/none && > > +export HOME && > > What is the reason why this test needs to move HOME away from > TRASH_DIRECTORY (set in t/test-lib.sh)? This is to make sure that no user.name nor user.email is configured. That was my idea, hence I answer your question. Ciao, Dscho > > > +unset GIT_AUTHOR_NAME && > > +unset GIT_AUTHOR_EMAIL && > > +unset GIT_COMMITTER_NAME && > > +unset GIT_COMMITTER_EMAIL && > > +test_must_fail git config user.email && > > +echo changed >1.t && > > + git stash > > +) > > +' > > + > > test_done >
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
Slavica writes: > +test_expect_failure 'stash with HOME as non-existing directory' ' > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +( > +HOME=$(pwd)/none && > +export HOME && What is the reason why this test needs to move HOME away from TRASH_DIRECTORY (set in t/test-lib.sh)? > +unset GIT_AUTHOR_NAME && > +unset GIT_AUTHOR_EMAIL && > +unset GIT_COMMITTER_NAME && > +unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && > +echo changed >1.t && > + git stash > +) > +' > + > test_done
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
On Tue, Oct 23, 2018 at 12:31 PM Slavica wrote: > This is part of enhancement request that ask for `git stash` to work even if > `user.name` is not configured. > The issue is discussed here: > https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. As Christian mentioned already, it's best to try to describe the issue succinctly in the commit message so readers can understand it without chasing a link. For this simple case, it should be sufficient to explain that, due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. > Signed-off-by: Slavica > --- > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > @@ -1156,4 +1156,21 @@ test_expect_success 'stash -- works with > binary files' ' > +test_expect_failure 'stash with HOME as non-existing directory' ' The purpose of this test is to demonstrate that git-stash has an undesirable requirement that 'user.name' and 'user.email' be set. The test title should reflect that. So, instead of talking about non-existent HOME (which is just an implementation detail of the test), a better test title would be something like "stash works when user.name and user.email are not set". > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +( > +HOME=$(pwd)/none && > +export HOME && > +unset GIT_AUTHOR_NAME && Use sane_unset() for all of these rather than bare 'unset'. > +unset GIT_AUTHOR_EMAIL && > +unset GIT_COMMITTER_NAME && > +unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && > +echo changed >1.t && > + git stash Christian already mentioned the odd indentation. > +) > +'
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote: > > This is part of enhancement request that ask for `git stash` to work even if > `user.name` is not configured. > The issue is discussed here: > https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. We prefer commit messages that contain as much as possible all the information necessary to understand the patch without links to other places. It seems that only this email from you reached me. Did you send other emails for patches 2/3 and 3/3? [...] > +( > +HOME=$(pwd)/none && > +export HOME && > +unset GIT_AUTHOR_NAME && > +unset GIT_AUTHOR_EMAIL && > +unset GIT_COMMITTER_NAME && > +unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && > +echo changed >1.t && > + git stash It seems that the above line is not indented like the previous ones. > +) > +' Thanks for contributing, Christian.
[PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured. The issue is discussed here: https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. Signed-off-by: Slavica --- t/t3903-stash.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 9e06494ba0..9ff34a65bc 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1156,4 +1156,21 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash with HOME as non-existing directory' ' +test_commit 1 && +test_config user.useconfigonly true && +test_config stash.usebuiltin true && +( +HOME=$(pwd)/none && +export HOME && +unset GIT_AUTHOR_NAME && +unset GIT_AUTHOR_EMAIL && +unset GIT_COMMITTER_NAME && +unset GIT_COMMITTER_EMAIL && +test_must_fail git config user.email && +echo changed >1.t && + git stash +) +' + test_done -- 2.19.1.windows.1
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
Hi Eric,
On Tue, 16 Oct 2018, Eric Sunshine wrote:
> On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
> wrote:
> > On Mon, 15 Oct 2018, Eric Sunshine wrote:
> > > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> > > wrote:
> > > > + len = ARRAY_SIZE(wbuffer);
> > > > + if (GetUserNameExW(type, wbuffer, )) {
> > > > + char *converted = xmalloc((len *= 3));
> >
> > Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call
> > needs to receive `len + 1` to accommodate for the trailing NUL. Will fix,
> > too.
>
> Or, xmallocz() (note the "z") which gives you overflow-checking of the
> +1 for free.
Very good point! Thanks for the suggestion,
Dscho
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
wrote:
> On Mon, 15 Oct 2018, Eric Sunshine wrote:
> > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> > wrote:
> > > + len = ARRAY_SIZE(wbuffer);
> > > + if (GetUserNameExW(type, wbuffer, )) {
> > > + char *converted = xmalloc((len *= 3));
>
> Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call
> needs to receive `len + 1` to accommodate for the trailing NUL. Will fix,
> too.
Or, xmallocz() (note the "z") which gives you overflow-checking of the
+1 for free.
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
Hi Eric,
On Mon, 15 Oct 2018, Eric Sunshine wrote:
> On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> wrote:
> > We do have the excellent GetUserInfoEx() function to obtain more
> > detailed information of the current user (if the user is part of a
> > Windows domain); Let's use it.
> > [...]
> > Signed-off-by: Johannes Schindelin
> > ---
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void)
> > +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
> > +{
> > + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
> > + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
> > + static wchar_t wbuffer[1024];
>
> Does this need to be static? It's not being returned to the caller.
It does not. Fixed.
> > + len = ARRAY_SIZE(wbuffer);
> > + if (GetUserNameExW(type, wbuffer, )) {
> > + char *converted = xmalloc((len *= 3));
Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call
needs to receive `len + 1` to accommodate for the trailing NUL. Will fix,
too.
Ciao,
Dscho
> > + if (xwcstoutf(converted, wbuffer, len) >= 0)
> > + return converted;
> > + free(converted);
> > + }
>
> If xwcstoutf() fails, 'converted' is freed; otherwise, the allocated
> 'converted' is stored in the caller's statically held 'passwd' struct.
> Okay.
>
> > + return NULL;
> > +}
>
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
wrote:
> We do have the excellent GetUserInfoEx() function to obtain more
> detailed information of the current user (if the user is part of a
> Windows domain); Let's use it.
> [...]
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void)
> +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
> +{
> + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
> + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
> + static wchar_t wbuffer[1024];
Does this need to be static? It's not being returned to the caller.
> + len = ARRAY_SIZE(wbuffer);
> + if (GetUserNameExW(type, wbuffer, )) {
> + char *converted = xmalloc((len *= 3));
> + if (xwcstoutf(converted, wbuffer, len) >= 0)
> + return converted;
> + free(converted);
> + }
If xwcstoutf() fails, 'converted' is freed; otherwise, the allocated
'converted' is stored in the caller's statically held 'passwd' struct.
Okay.
> + return NULL;
> +}
[PATCH 2/3] getpwuid(mingw): provide a better default for the user name
From: Johannes Schindelin
We do have the excellent GetUserInfoEx() function to obtain more
detailed information of the current user (if the user is part of a
Windows domain); Let's use it.
Suggested by Lutz Roeder.
To avoid the cost of loading Secur32.dll (even lazily, loading DLLs
takes a non-neglibile amount of time), we use the established technique
to load DLLs only when, and if, needed.
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 32 +++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 597781b370..623ff5daf5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,6 +5,7 @@
#include "../strbuf.h"
#include "../run-command.h"
#include "../cache.h"
+#include "win32/lazyload.h"
#define HCAST(type, handle) ((type)(intptr_t)handle)
@@ -1798,6 +1799,33 @@ int mingw_getpagesize(void)
return si.dwAllocationGranularity;
}
+/* See https://msdn.microsoft.com/en-us/library/windows/desktop/ms724435.aspx
*/
+enum EXTENDED_NAME_FORMAT {
+ NameDisplay = 3,
+ NameUserPrincipal = 8
+};
+
+static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
+{
+ DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
+ enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
+ static wchar_t wbuffer[1024];
+ DWORD len;
+
+ if (!INIT_PROC_ADDR(GetUserNameExW))
+ return NULL;
+
+ len = ARRAY_SIZE(wbuffer);
+ if (GetUserNameExW(type, wbuffer, )) {
+ char *converted = xmalloc((len *= 3));
+ if (xwcstoutf(converted, wbuffer, len) >= 0)
+ return converted;
+ free(converted);
+ }
+
+ return NULL;
+}
+
struct passwd *getpwuid(int uid)
{
static unsigned initialized;
@@ -1816,7 +1844,9 @@ struct passwd *getpwuid(int uid)
p = xmalloc(sizeof(*p));
p->pw_name = user_name;
- p->pw_gecos = "unknown";
+ p->pw_gecos = get_extended_user_info(NameDisplay);
+ if (!p->pw_gecos)
+ p->pw_gecos = "unknown";
p->pw_dir = NULL;
initialized = 1;
--
gitgitgadget
Re: user name
Thanks for the info, yes your are correct it was for the git config not github. I'm brand new to this and wasn't sure if a real name was required vs just a username. I know is some cases official names are required and didn't want to use the wrong one to start out with. I'm reading the help files/information currently and one of the first steps was to set this up. I'll set up github separately. Thanks for the info! Dennis On Tue, May 22, 2018 at 9:45 AM, Christian Couder <christian.cou...@gmail.com> wrote: > On Tue, May 22, 2018 at 3:06 PM, Dennis Powless <claven...@gmail.com> wrote: >> Is it customary to use your real name or a user name when registering to GIT? > > I guess you are talking about using `git config --global user.name > "XXX YYY"`. (Though maybe you are talking about github.com > registration, but in this case you should not have sent this request > to this mailing list.) > > When you use git config as described above, this is not a real > registration. You just configure Git on your machine (only your > ~/.gitconfig is changed), so that Git knows what to put in the > "author" field of the commits you create. > > You are free to use whatever you want but some projects might ask > contributors to put their real name in the author fields of the > commits (and sometimes in other places too, like in the > "Signed-off-by" in the commit messages).
Re: user name
On Tue, May 22, 2018 at 3:06 PM, Dennis Powless <claven...@gmail.com> wrote: > Is it customary to use your real name or a user name when registering to GIT? I guess you are talking about using `git config --global user.name "XXX YYY"`. (Though maybe you are talking about github.com registration, but in this case you should not have sent this request to this mailing list.) When you use git config as described above, this is not a real registration. You just configure Git on your machine (only your ~/.gitconfig is changed), so that Git knows what to put in the "author" field of the commits you create. You are free to use whatever you want but some projects might ask contributors to put their real name in the author fields of the commits (and sometimes in other places too, like in the "Signed-off-by" in the commit messages).
user name
Is it customary to use your real name or a user name when registering to GIT? Dennis
[PATCH v2 3/3] t5500: show user name and host in diag-url
The URL for ssh may have include a username before the hostname,
like ssh://user@host/repo.
When literal IPV6 addresses are used together with a username,
the substring user@[::1] must be converted into user@::1.
Make that conversion visible for the user, and write userandhost
in the diagnostics
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 35 +++
t/t5500-fetch-pack.sh | 51 +--
2 files changed, 56 insertions(+), 30 deletions(-)
diff --git a/connect.c b/connect.c
index b608976..84f8156 100644
--- a/connect.c
+++ b/connect.c
@@ -675,7 +675,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, hostandport, path);
- if (flags CONNECT_DIAG_URL) {
+ if ((flags CONNECT_DIAG_URL) (protocol != PROTO_SSH)) {
printf(Diag: url=%s\n, url ? url : NULL);
printf(Diag: protocol=%s\n, prot_name(protocol));
printf(Diag: hostandport=%s\n, hostandport ? hostandport :
NULL);
@@ -719,18 +719,29 @@ struct child_process *git_connect(int fd[2], const char
*url,
get_host_and_port(ssh_host, port);
if (!port)
port = get_port(ssh_host);
-
- if (!ssh) ssh = ssh;
-
- argv_array_push(conn-args, ssh);
- if (putty !strcasestr(ssh, tortoiseplink))
- argv_array_push(conn-args, -batch);
- if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(conn-args, putty ? -P :
-p);
- argv_array_push(conn-args, port);
+ if (flags CONNECT_DIAG_URL) {
+ printf(Diag: url=%s\n, url ? url : NULL);
+ printf(Diag: protocol=%s\n,
prot_name(protocol));
+ printf(Diag: userandhost=%s\n, ssh_host ?
ssh_host : NULL);
+ printf(Diag: port=%s\n, port ? port : NONE);
+ printf(Diag: path=%s\n, path ? path : NULL);
+
+ free(hostandport);
+ free(path);
+ return NULL;
+ } else {
+ if (!ssh) ssh = ssh;
+
+ argv_array_push(conn-args, ssh);
+ if (putty !strcasestr(ssh, tortoiseplink))
+ argv_array_push(conn-args, -batch);
+ if (port) {
+ /* P is for PuTTY, p is for OpenSSH */
+ argv_array_push(conn-args, putty ?
-P : -p);
+ argv_array_push(conn-args, port);
+ }
+ argv_array_push(conn-args, ssh_host);
}
- argv_array_push(conn-args, ssh_host);
} else {
/* remove repo-local variables from the environment */
conn-env = local_repo_env;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 5b2b1c2..bd37f04 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -541,13 +541,30 @@ check_prot_path () {
test_cmp expected actual
}
-check_prot_host_path () {
- cat expected -EOF
+check_prot_host_port_path () {
+ local diagport
+ case $2 in
+ *ssh*)
+ pp=ssh
+ uah=userandhost
+ ehost=$(echo $3 | tr -d [])
+ diagport=Diag: port=$4
+ ;;
+ *)
+ pp=$p
+ uah=hostandport
+ ehost=$(echo $3$4 | sed -e s/22$/:22/ -e s/NONE//)
+ diagport=
+ ;;
+ esac
+ cat exp -EOF
Diag: url=$1
- Diag: protocol=$2
- Diag: hostandport=$3
- Diag: path=$4
+ Diag: protocol=$pp
+ Diag: $uah=$ehost
+ $diagport
+ Diag: path=$5
EOF
+ grep -v ^$ exp expected
git fetch-pack --diag-url $1 actual
test_cmp expected actual
}
@@ -557,22 +574,20 @@ do
# git or ssh with scheme
for p in ssh+git git+ssh git ssh
do
- for h in host host:12 [::1] [::1]:23
+ for h in host user@host user@[::1] user@::1
do
- case $p in
- *ssh*)
- pp=ssh
- ;;
- *)
- pp=$p
- ;;
- esac
[PATCH 3/3] t5500: Show user name and host in diag-url
The URL for ssh may have include a username before the hostname,
like ssh://user@host/repo.
When literal IPV6 addresses are used together with a username,
the substring user@[::1] must be converted into user@::1.
Make that conversion visible for the user, and write userandhost
in the diagnostics
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 35 +++
t/t5500-fetch-pack.sh | 51 +--
2 files changed, 56 insertions(+), 30 deletions(-)
diff --git a/connect.c b/connect.c
index b608976..84f8156 100644
--- a/connect.c
+++ b/connect.c
@@ -675,7 +675,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, hostandport, path);
- if (flags CONNECT_DIAG_URL) {
+ if ((flags CONNECT_DIAG_URL) (protocol != PROTO_SSH)) {
printf(Diag: url=%s\n, url ? url : NULL);
printf(Diag: protocol=%s\n, prot_name(protocol));
printf(Diag: hostandport=%s\n, hostandport ? hostandport :
NULL);
@@ -719,18 +719,29 @@ struct child_process *git_connect(int fd[2], const char
*url,
get_host_and_port(ssh_host, port);
if (!port)
port = get_port(ssh_host);
-
- if (!ssh) ssh = ssh;
-
- argv_array_push(conn-args, ssh);
- if (putty !strcasestr(ssh, tortoiseplink))
- argv_array_push(conn-args, -batch);
- if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(conn-args, putty ? -P :
-p);
- argv_array_push(conn-args, port);
+ if (flags CONNECT_DIAG_URL) {
+ printf(Diag: url=%s\n, url ? url : NULL);
+ printf(Diag: protocol=%s\n,
prot_name(protocol));
+ printf(Diag: userandhost=%s\n, ssh_host ?
ssh_host : NULL);
+ printf(Diag: port=%s\n, port ? port : NONE);
+ printf(Diag: path=%s\n, path ? path : NULL);
+
+ free(hostandport);
+ free(path);
+ return NULL;
+ } else {
+ if (!ssh) ssh = ssh;
+
+ argv_array_push(conn-args, ssh);
+ if (putty !strcasestr(ssh, tortoiseplink))
+ argv_array_push(conn-args, -batch);
+ if (port) {
+ /* P is for PuTTY, p is for OpenSSH */
+ argv_array_push(conn-args, putty ?
-P : -p);
+ argv_array_push(conn-args, port);
+ }
+ argv_array_push(conn-args, ssh_host);
}
- argv_array_push(conn-args, ssh_host);
} else {
/* remove repo-local variables from the environment */
conn-env = local_repo_env;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 5b2b1c2..bd37f04 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -541,13 +541,30 @@ check_prot_path () {
test_cmp expected actual
}
-check_prot_host_path () {
- cat expected -EOF
+check_prot_host_port_path () {
+ local diagport
+ case $2 in
+ *ssh*)
+ pp=ssh
+ uah=userandhost
+ ehost=$(echo $3 | tr -d [])
+ diagport=Diag: port=$4
+ ;;
+ *)
+ pp=$p
+ uah=hostandport
+ ehost=$(echo $3$4 | sed -e s/22$/:22/ -e s/NONE//)
+ diagport=
+ ;;
+ esac
+ cat exp -EOF
Diag: url=$1
- Diag: protocol=$2
- Diag: hostandport=$3
- Diag: path=$4
+ Diag: protocol=$pp
+ Diag: $uah=$ehost
+ $diagport
+ Diag: path=$5
EOF
+ grep -v ^$ exp expected
git fetch-pack --diag-url $1 actual
test_cmp expected actual
}
@@ -557,22 +574,20 @@ do
# git or ssh with scheme
for p in ssh+git git+ssh git ssh
do
- for h in host host:12 [::1] [::1]:23
+ for h in host user@host user@[::1] user@::1
do
- case $p in
- *ssh*)
- pp=ssh
- ;;
- *)
- pp=$p
- ;;
- esac
Authentication with e-mail address as user name for HTTPS remote
Hello, I run into a problem with command line git on Linux. The remote git server I try to clone from uses HTTPS as a protocol and requires full fledged e-mail addresses for a user name in its authentication. In TortoiseGit (with winstore) or SourceTree, the user name and password are asked and stored. But on the command line I'm stuck, unable to provide the user name. Here is what happens. First try cloning without specifying the user name: ---8--- $ git clone https://git.server.com/git/test.git Initialized empty Git repository in /tmp/git/test/.git/ error: The requested URL returned error: 401 Authorization Required while accessing https://git.server.com/git/test.git/nfo/refs fatal: HTTP request failed ---8--- I couldn't find a --username flag or something similar for the git command, so my next try was to incorporate the user name in the URL, basic auth style. The repo URL looks something like this: https://user.em...@emaildomain.com@git.server.com/git/test.git Note the double @ there, it is bound to cause trouble. This is what happened: ---8--- $ git clone https://user.em...@emaildomain.com@git.server.com/git/test.git Initialized empty Git repository in /tmp/git/test/.git/ Password: error: Couldn't resolve host 'emaildomain@git.server.com' while accessing https://user.em...@emaildomain.com@git.server.com/git/test.git/info/refs fatal: HTTP request failed ---8--- It appears the @ was picked up as an indication that the URL contains the username, however the URL was split at the wrong position. The splitting algorithm doesn't seem to take into account that a user name might contain an @. My request: please modify command line git to support full fledged e-mail addresses as user names in HTTPS requests. I'm not a C adept, still I browsed the code a bit and bumped into transport.c's transport_anonymize_url method. I think that code could be modified to do a better job of splitting the URL, which might then avoid problems down the line. Thanks for reading this far. Regards, Patrick -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Authentication with e-mail address as user name for HTTPS remote
On Sat, Aug 31, 2013 at 08:52:06AM +0200, Patrick Atoon wrote: Here is what happens. First try cloning without specifying the user name: ---8--- $ git clone https://git.server.com/git/test.git Initialized empty Git repository in /tmp/git/test/.git/ error: The requested URL returned error: 401 Authorization Required while accessing https://git.server.com/git/test.git/nfo/refs fatal: HTTP request failed That should be prompting you. What version of git are you using? I couldn't find a --username flag or something similar for the git command, so my next try was to incorporate the user name in the URL, basic auth style. Yes, that's the correct way to do it from the command line. If you are running v1.7.9 and later, you can also put this in your config: [credential https://git.server.com;] username = user.em...@emaildomain.com to automatically use that username for the particular domain. https://user.em...@emaildomain.com@git.server.com/git/test.git Note the double @ there, it is bound to cause trouble. Yes. You probably want to escape it like: https://user.email%40emaildomain@git.server.com/git/test.git In theory we could parse from the right-hand side of the hostname and realize that only the right-most @ is the username separator (under the assumption that hostnames can never contain an @). I don't know if that would violate the standard or not. However, it is not even git that does the actual parsing in this case, but rather libcurl. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
