Re: [PATCH v2 2/3] grep: make PCRE2 aware of custom allocator

[Junio C Hamano]

"Carlo Marcelo Arenas Belón via GitGitGadget"
 writes:

>  builtin/grep.c |  1 +
>  grep.c | 34 +-
>  grep.h |  1 +
>  3 files changed, 35 insertions(+), 1 deletion(-)

>  
> +#if defined(USE_LIBPCRE2)
> + if (!pcre2_global_context)
> + pcre2_global_context = pcre2_general_context_create(
> + pcre2_malloc, pcre2_free, NULL);
> +#endif

This part should use the same "#ifdef" as the other one which is a
minor nit, for consistency.  I do not care too deeply which way we
unify, but we should stick to one.

> +
>  #ifdef USE_LIBPCRE1
>   pcre_malloc = malloc;
>   pcre_free = free;

Other than that, all 3 patches look sensible, and they certainly got
simplified by dropping the conditional ;-).

Re: [PATCH v1] config/branch: state that .merge is the remote ref

[Junio C Hamano]

Philip Oakley  writes:

>  branch..merge::
>   Defines, together with branch..remote, the upstream branch
> - for the given branch. It tells 'git fetch'/'git pull'/'git rebase' which
> + for the given branch. It defines the branch name _on the remote_,
> + which may be different from the local branch name.

While I do agree with the goal of make things more clear, I'd avoid
being overly redundant and giving irrelevant information (e.g. the
copy you have locally may be made under arbitrary name that is
different from the name used at the remote).  After all, the users
do not even need to know about the remote-tracking branch to
understand this naming mechanism.

Perhaps something along this line instead:

The name of the branch at the remote `branch..remote` that
is used as the upstream branch for the given branch.  It tells
`git fetch`, etc., which branch to merge and ...

Re: email as a bona fide git transport

[Greg KH]

On Thu, Oct 17, 2019 at 04:45:32PM -0400, Konstantin Ryabitsev wrote:
> On Thu, Oct 17, 2019 at 01:43:43PM -0700, Greg KH wrote:
> > > I wonder if it'd be also possible to then embed gpg signatures over
> > > send-mail payloads so as they can be transparently transferred to the
> > > commit.
> > 
> > That's a crazy idea.  It would be nice if we could do that, I like it :)
> 
> It could only possibly work if nobody ever adds their own "Signed-Off-By" or
> any other bylines. I expect this is a deal-breaker for most maintainers.

Yeah it is :(

But, if we could just have the signature on the code change, not the
changelog text, that would help with that issue.

thanks,

rgeg k-h

Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" 
writes:

> From: Johannes Schindelin 
>
> Ever since worktrees were introduced, the `git_path()` function _really_
> needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
> specific to the worktree). However, the wrong path is returned for
> `logs/HEAD.lock`.
>
> This does not matter as long as the Git executable is doing the asking,
> as the path for that `index.lock` file is constructed from
> `git_path("index")` by appending the `.lock` suffix.

Is this still git_path("index") or is it now HEAD?

> Side note: Git GUI _does_ ask for `index.lock`, but that is already
> resolved correctly.

Is that s/but/and/?

> diff --git a/path.c b/path.c
> index e3da1f3c4e..ff85692b45 100644
> --- a/path.c
> +++ b/path.c
> @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, 
> match_fn fn,
>   int result;
>   struct trie *child;
>  
> - if (!*key) {
> + if (!*key || !strcmp(key, ".lock")) {

We only do strcmp for the tail part at the end of the path, so this
should probably OK from performance point of view but semantically
it is not very satisfying to see a special case for a single .suffix
this deep in the callchain.  I wonder if it is nicer to have the
higher level callers notice ".lock" or whatever other suffixes they
care about and ask the lower layer for a key with the suffix
stripped?

Will queue.

Thanks.

Re: git smart http + apache mod_auth_openidc

[Ralph Ewig]

Understood (and agree).

We do use git for source code (where we use SSH 
and key authentication for CI/CD), but also for 
configuration control of other files like 
financial reports, engineering drawings, etc. 
where access is via HTTPS.  In that 2nd group the 
challenge is to make it as "not coding like" as 
possible so the non-developer crowd isn't scared off.

Since we use trac for project management company 
wide (all verticals), my latest idea is to 
intercept the git http request on the server side 
to authenticate against the trac session info 
stored in the db (using a custom php script),  and 
then pass it on to git-http-backend or throw an 
error prompting the user to sign into trac first. 
Most users are signed into trac 24/7 anyway since 
its central to our workflow.

in the end paying the extra fee for MS Domain 
Services to get SSO via LDAP or Kerberos might be 
the right answer though - just trying to be 
scrappy if I can since it's an early stage 
startup. Nonetheless really appreciate the 
exchange of ideas!

Ralph


On 10/17/2019 3:55 PM, brian m. carlson wrote:
> On 2019-10-17 at 14:33:38, Ralph Ewig wrote:
>> Quick follow up question: can the git client pass
>> a token read from a cookie with a request? That
>> would enable users to sign-in via a browser, store
>> the cookie, and then use that as the access token
>> to authenticate a git request.
> Git has an option, http.cookieFile, that can read a cookie from a file,
> yes.  That does, of course, require that you're able to put the cookie
> in a file from a web browser.  I'm not aware of any web browsers that
> easily provide an option to dump cookies into a file.
>
> Also, just as a note, this approach definitely won't work for automated
> systems you have, such as CI systems.  That's why I suggested Kerberos:
> because you can have services that have principals and you can use those
> credentials in your CI systems to access code and run jobs.
>
> Clearly you know your infrastructure and users better than I do, but I
> don't recommend having a web-based sign-on as your only form of
> authentication for a Git server.  It's going to cause a lot of pain and
> inconvenience on all sides.

Re: [PATCH v3 08/13] graph: tidy up display of left-skewed merges

[Junio C Hamano]

Derrick Stolee  writes:

> I hit this very situation recently when I was experimenting with
> 'git fast-import' and accidentally created many parallel, independent
> histories. Running "git log --graph --all --simplify-by-decoration"
> made it look like all the refs were in a line, but they were not.
> (The one way I knew something was up: the base commits also appeared
> without a decoration. That was the only clue that the histories did
> not continue in a line.)
>
>> 
>> and the fact that B and A do not share parent-child relationships is
>> lost.  An easy way to show that would be to draw the bottom three
>> lines of the full history output we saw earlier:
>> 
>> | * e6277a9 C
>> | * 13ae9b2 B
>> * afee005 A
>> 
>> either with or without the vertical bar to imply that A may have a
>> child.
> The natural extension of this would be multiple columns:
>
>  | | | | | *
>  | | | | *
>  | | | *
>  | | *
>  | *
>  *

After sleeping over it, I now think we shouldn't draw lines that
imply a child for each of these commits, as we haven't seen, but
I agree that this can be extended to 3 or more roots.

Re: [PATCH 2/2] notes: fix minimum number of parameters to "copy" subcommand

[Junio C Hamano]

Doan Tran Cong Danh  writes:

>  Documentation/git-notes.txt |  6 +++---
>  builtin/notes.c |  2 +-
>  t/t3301-notes.sh| 40 +++--
>  3 files changed, 42 insertions(+), 6 deletions(-)

Thanks, makes sense.

Re: [PATCH 1/2] t3301: test diagnose messages for too few/many paramters

[Junio C Hamano]

Doan Tran Cong Danh  writes:

> If we accidentally lifted the check inside our code base, the test may
> still failed because the provided parameter is not a valid ref.

Makes sense.  Another option would be to use a valid ref to make
sure there are no other possible reason for the command to fail,
which would make the test robust against us accidentally switching
the order of the check to see if they are refs first and then
complain about too many or too few arguments ;-)  But I think what
we have here is fine as-is (I'll copy-edit the proposed log message
for grammos, though).

Thanks.

>
> Correct it.
>
> Signed-off-by: Doan Tran Cong Danh 
> ---
>  t/t3301-notes.sh | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index d3fa298c6a..d7767e4438 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1167,8 +1167,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides 
> config' '
>  '
>  
>  test_expect_success 'git notes copy diagnoses too many or too few 
> parameters' '
> - test_must_fail git notes copy &&
> - test_must_fail git notes copy one two three
> + test_must_fail git notes copy 2>error &&
> + test_i18ngrep "too few parameters" error &&
> + test_must_fail git notes copy one two three 2>error &&
> + test_i18ngrep "too many parameters" error
>  '
>  
>  test_expect_success 'git notes get-ref expands refs/heads/master to 
> refs/notes/refs/heads/master' '

Re: Commits with --no-verify option

[brian m. carlson]

On 2019-10-17 at 08:56:34, Manoj Sterex wrote:
> Hi all,
> 
> Currently, AFAIK, there is no way to know if a commit was done with or
> without using the '--no-verify' option. That is, git does not track if
> hooks were skipped when the commit happened.
> 
> Is there some way to track this in the log?

No, there isn't, and even if there were, it would be trivial to bypass
(such as by using git commit-tree, which does not run hooks).

You haven't elaborated on why you want this, but typically the reason
people ask for this is that they do some sort of data checking (testing
or linting) in the pre-commit hook.  Since that code runs on the
author's system and is trivially bypassed (by simply deleting the hook),
it isn't a good way to enforce policy; usually that should be done in a
test or CI infrastructure.

In addition, it's common for advanced users to make a series of
temporary commits to save state and then squash them later.  It wouldn't
be reasonable for a pre-commit hook to enforce certain policies, such as
a commit message syntax policy, in such a case, so --no-verify exists
for users who know what they're doing and would like to bypass standard
checking.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 4.5/12] t5520: replace test -f with test-lib functions

[Eric Sunshine]

On Thu, Oct 17, 2019 at 7:35 PM Denton Liu  wrote:
> Although `test -f` has the same functionality as test_path_is_file(), in
> the case where test_path_is_file() fails, we get much better debugging
> information.
>
> Replace `test -f` with test_path_is_file() so that future developers
> will have a better experience debugging these test cases. Also, in the
> case of `! test -f`, replace it with test_path_is_missing().
>
> Signed-off-by: Denton Liu 
> ---
> I just realised that test_path_is_missing() is a much better replacement
> than `test_must_fail test_path_is_file`.

That depends upon context...

> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -99,7 +99,7 @@ test_expect_success 'pulling into void must not create an 
> octopus' '
> -   ! test -f file
> +   test_path_is_missing file

There is a semantic difference between checking that "file" is not a
_file_ versus checking that the a path itself (which may be a
directory or a "special file") does not exist. In this particular
test, the difference doesn't matter, so the conversion is sensible
enough, but it would save reviewers time if you, as author of the
patch, state that you carefully considered the distinction before
making each change.

Re: [PATCH v4 00/17] New sparse-checkout builtin and "cone" mode

[Jon Simons]

On 10/15/19 6:55 AM, Derrick Stolee via GitGitGadget wrote:

V4 UPDATE: Rebased on latest master to include ew/hashmap and
ds/include-exclude in the base.


I did a partial review of the v4 patches out of curiosity and I notice
in sparse-checkout.c there are a couple of unchecked `fopen` call sites
that could be converted to `xfopen` to die gracefully upon error, and
one unchecked `fdopen` that can likewise be `xfdopen`.


-Jon

Re: [PATCH 09/12] t5520: test single-line files by git with test_cmp

[Eric Sunshine]

On Thu, Oct 17, 2019 at 7:17 PM Denton Liu  wrote:
> In case an invocation of a Git command fails within the subshell, the
> failure will be masked. Replace the subshell with a file-redirection and
> a call to test_cmp.
>
> Signed-off-by: Denton Liu 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -622,10 +648,16 @@ test_expect_success 'pull --rebase fails on unborn 
> branch with staged changes' '
> git add staged-file &&
> -   test "$(git ls-files)" = staged-file &&
> +   echo staged-file >expect &&
> +   git ls-files >actual &&
> +   test_cmp expect actual &&
> test_must_fail git pull --rebase .. master 2>err &&
> -   test "$(git ls-files)" = staged-file &&
> -   test "$(git show :staged-file)" = staged-file &&
> +   echo staged-file >expect &&
> +   git ls-files >actual &&
> +   test_cmp expect actual &&
> +   echo staged-file >expect &&
> +   git show :staged-file >actual &&
> +   test_cmp expect actual &&

Nit: I'm not convinced that it makes sense to re-create the "expect"
file with the exact same content repeatedly in this one test. There is
some value in creating the file once and then _using_ it repeatedly
since that shows intent that the the result of these various commands
is not expected to change.

Re: [PATCH 08/12] t5520: use test_cmp_rev where possible

[Eric Sunshine]

On Thu, Oct 17, 2019 at 7:17 PM Denton Liu  wrote:
> In case an invocation of `git rev-list` fails within the subshell, the
> failure will be masked. Remove the subshell and use test_cmp_rev() so
> that failures can be discovered.
>
> Signed-off-by: Denton Liu 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -597,10 +597,10 @@ test_expect_success 'pull --rebase dies early with 
> dirty working directory' '
> test_must_fail git pull &&
> -   test "$COPY" = "$(git rev-parse --verify me/copy)" &&
> +   test_cmp_rev "$COPY" me/copy &&

This transformation doesn't look correct. COPY already holds the
result of a git-rev-parse invocation:

COPY="$(git rev-parse --verify me/copy)" &&

so passing it to test_cmp_rev() -- which applies its own git-rev-parse
invocation -- doesn't make sense.

> git checkout HEAD -- file &&
> git pull &&
> -   test "$COPY" != "$(git rev-parse --verify me/copy)"
> +   test_must_fail test_cmp_rev "$COPY" me/copy

As mentioned in my review of the other patch, this is not a valid use
of test_must_fail().

Re: [PATCH 07/12] t5520: replace test -{n,z} with test-lib functions

[Eric Sunshine]

On Thu, Oct 17, 2019 at 7:17 PM Denton Liu  wrote:
> Instead of using `test -n` or `test -z`, replace them respectively with
> invocations of test_file_not_empty() and test_must_be_empty().
>
> Signed-off-by: Denton Liu 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -206,15 +206,18 @@ test_expect_success 'fail if the index has unresolved 
> entries' '
> -   test -z "$(git ls-files -u)" &&
> +   git ls-files -u >unmerged &&
> +   test_must_be_empty unmerged &&

A better justification for these change (and all others in this patch)
is that the original threw away the exit code of the "git ls-files -u"
invocation, whereas the new code takes the exit code into account. The
commit message perhaps instead ought to focus on that benefit.

Re: [PATCH 04/12] t5520: replace test -f with test_path_is_file

[Eric Sunshine]

On Thu, Oct 17, 2019 at 7:17 PM Denton Liu  wrote:
> Although `test -f` has the same functionality as test_path_is_file(), in
> the case where test_path_is_file() fails, we get much better debugging
> information. Replace `test -f` with test_path_is_file so that future
> developers will have a better experience debugging these test cases.
>
> Signed-off-by: Denton Liu 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -99,7 +99,7 @@ test_expect_success 'pulling into void must not create an 
> octopus' '
> (
> cd cloned-octopus &&
> test_must_fail git pull .. master master &&
> -   ! test -f file
> +   test_must_fail test_path_is_file file
> )

According to t/README, this is not a valid use of test_must_fail(),
which is reserved for detecting failure of Git commands. Either use
'!' as the original code did, or perhaps use test_path_is_missing().

Re: git smart http + apache mod_auth_openidc

[brian m. carlson]

On 2019-10-17 at 14:33:38, Ralph Ewig wrote:
> Quick follow up question: can the git client pass
> a token read from a cookie with a request? That
> would enable users to sign-in via a browser, store
> the cookie, and then use that as the access token
> to authenticate a git request.

Git has an option, http.cookieFile, that can read a cookie from a file,
yes.  That does, of course, require that you're able to put the cookie
in a file from a web browser.  I'm not aware of any web browsers that
easily provide an option to dump cookies into a file.

Also, just as a note, this approach definitely won't work for automated
systems you have, such as CI systems.  That's why I suggested Kerberos:
because you can have services that have principals and you can use those
credentials in your CI systems to access code and run jobs.

Clearly you know your infrastructure and users better than I do, but I
don't recommend having a web-based sign-on as your only form of
authentication for a Git server.  It's going to cause a lot of pain and
inconvenience on all sides.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] doc: provide guidance on user.name format

[brian m. carlson]

On 2019-10-17 at 05:40:52, Jeff King wrote:
> On Thu, Oct 17, 2019 at 12:53:28AM +, brian m. carlson wrote:
> 
> > It's a frequent misconception that the user.name variable controls
> > authentication in some way, and as a result, beginning users frequently
> > attempt to change it when they're having authentication troubles.
> > Document that the convention is that this variable represents some form
> > of a human's personal name, although that is not required.  In addition,
> > address concerns about whether Unicode is supported.
> > 
> > Use the term "personal name" as this is likely to draw the intended
> > contrast, be applicable across cultures which may have different naming
> > conventions, and be easily understandable to people who do not speak
> > English as their first language.  Indicate that "some form" is
> > conventionally used, as people may use a nickname or preferred name
> > instead of a full legal name.
> > 
> > Point users who may be confused about authentication to an appropriate
> > configuration option instead.
> 
> I think this is a good distinction to draw, but...
> 
> >  Documentation/git-commit-tree.txt | 6 ++
> >  1 file changed, 6 insertions(+)
> 
> ...I was surprised to see it here, where I think most users wouldn't
> find it. Would it make sense in git-commit(1), or in the description of
> the user.name config?

So the user.name config description points to git-commit-tree(1), which
describes these in detail, which is why I put it there.  I agree that
it's not a super discoverable place, since I don't know anyone that
actually uses git commit-tree these days.  git-commit(1) doesn't
describe these options at all.

There are, of course, options.  I can add this text into the `user.name`
option in git-config(5) nevertheless, which will likely be more
discoverable, but it will split the documentation on those into two
separate locations.  Or we can leave it in git-commit-tree(1) anyway to
keep it together.

Since you and Junio think this is an odd place (and I agree), I think
it's fine to separate the text out, and I'll reroll with that change
unless someone speaks up strongly against it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[Johannes Schindelin]

Hi Gábor,

On Wed, 16 Oct 2019, SZEDER Gábor wrote:

> On Wed, Oct 16, 2019 at 07:07:17AM +, Johannes Schindelin via 
> GitGitGadget wrote:
> > From: Johannes Schindelin 
> >
> > Ever since worktrees were introduced, the `git_path()` function _really_
> > needed to be called e.g. to get at the `index`. However, the wrong path
> > is returned for `index.lock`.
>
> Could you give an example where it returns the wrong path for
> 'index.lock'?

Oh wow, this was a left-over from an early draft, before I got the
regression test to work... What I meant was of course logs/HEAD.lock.
Will fix.

> I tried to reproduce this issue in a working tree, but
> no matter what I've tried, 'git rev-parse --git-dir index.lock' always
> returned the right path.

With `s/--git-dir/--git-path/`, I agree.

> > This does not matter as long as the Git executable is doing the asking,
> > as the path for that `index.lock` file is constructed from
> > `git_path("index")` by appending the `.lock` suffix.
> >
> > However, Git GUI just learned to use `--git-path` instead of appending
> > relative paths to what `git rev-parse --git-dir` returns (and as a
> > consequence not only using the correct hooks directory, but also using
> > the correct paths in worktrees other than the main one). And one of the
> > paths it is looking for is... you guessed it... `index.lock`.
> >
> > So let's make that work as script writers would expect it to.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  path.c   |  4 ++--
> >  t/t1500-rev-parse.sh | 15 +++
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..ff85692b45 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char 
> > *key, match_fn fn,
> > int result;
> > struct trie *child;
> >
> > -   if (!*key) {
> > +   if (!*key || !strcmp(key, ".lock")) {
> > /* we have reached the end of the key */
> > if (root->value && !root->len)
> > return fn(key, root->value, baton);
> > @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char 
> > *key, match_fn fn,
> >
> > /* Matched the entire compressed section */
> > key += i;
> > -   if (!*key)
> > +   if (!*key || !strcmp(key, ".lock"))
> > /* End of key */
> > return fn(key, root->value, baton);
> >
> > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> > index 01abee533d..d318a1eeef 100755
> > --- a/t/t1500-rev-parse.sh
> > +++ b/t/t1500-rev-parse.sh
> > @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
> > test_cmp expect actual
> >  '
> >
> > +test_expect_success 'git-path in worktree' '
> > +   test_tick &&
> > +   git commit --allow-empty -m empty &&
> > +   git worktree add --detach wt &&
> > +   test_write_lines >expect \
> > +   "$(pwd)/.git/worktrees/wt/logs/HEAD" \
> > +   "$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
> > +   "$(pwd)/.git/worktrees/wt/index" \
> > +   "$(pwd)/.git/worktrees/wt/index.lock" &&
> > +   git -C wt rev-parse >actual \
> > +   --git-path logs/HEAD --git-path logs/HEAD.lock \
> > +   --git-path index --git-path index.lock &&
> > +   test_cmp expect actual
>
> Without the fix applied this test fails with:
>
>   + test_cmp expect actual
>   --- expect  2019-10-16 10:20:31.047229423 +
>   +++ actual  2019-10-16 10:20:31.051229519 +
>   @@ -1,4 +1,4 @@
>/home/szeder/src/git/t/trash 
> directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD
>   -/home/szeder/src/git/t/trash 
> directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD.lock
>   +/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/logs/HEAD.lock
>/home/szeder/src/git/t/trash 
> directory.t1500-rev-parse/.git/worktrees/wt/index
>/home/szeder/src/git/t/trash 
> directory.t1500-rev-parse/.git/worktrees/wt/index.lock
>   error: last command exited with $?=1
>
> So the path of 'index.lock' seems to be fine already, it's the path of
> the lockfile for HEAD's reflog that's indeed wrong and makes the test
> fail.

Indeed, and this makes this patch much less important than I previosly
thought. It's not like it would break Git GUI in worktrees, which is
what I thought, which in turn is the reason I sent this so close to
-rc0.

> On a related note, I'm not sure whether the path of the reflogs
> directory is right while in a different working tree...  Both with and
> without this patch I get a path pointing to the main working tree:
>
>   $ ./git -C WT/ rev-parse --git-path logs
>   /home/szeder/src/git/.git/logs
>
> However, I'm not sure what the right path should be in the first
> place, given that each working tree has its own 'logs' directory, but
> only for HEAD's reflog, while everything else goes to the main working
> tree's 'logs' directory.

It's like Junio said, the reflog for `HEAD` is special because `HEAD` is
s

Re: email as a bona fide git transport

[Konstantin Ryabitsev]

On Thu, Oct 17, 2019 at 01:43:43PM -0700, Greg KH wrote:

I wonder if it'd be also possible to then embed gpg signatures over
send-mail payloads so as they can be transparently transferred to the
commit.


That's a crazy idea.  It would be nice if we could do that, I like it 
:)


It could only possibly work if nobody ever adds their own 
"Signed-Off-By" or any other bylines. I expect this is a deal-breaker 
for most maintainers.


-K

Re: email as a bona fide git transport

[Greg KH]

On Wed, Oct 16, 2019 at 10:45:19AM -0400, Santiago Torres Arias wrote:
> Hi Willy, Vegard.
> 
> On Wed, Oct 16, 2019 at 01:10:09PM +0200, Willy Tarreau wrote:
> > Hi Vegard,
> > 
> > On Wed, Oct 16, 2019 at 12:22:54PM +0200, Vegard Nossum wrote:
> > > (cross-posted to git, LKML, and the kernel workflows mailing lists.)
> > > 
> > > Hi all,
> > > 
> > > I've been following Konstantin Ryabitsev's quest for better development
> > > and communication tools for the kernel [1][2][3], and I would like to
> > > propose a relatively straightforward idea which I think could bring a
> > > lot to the table.
> > > 
> > > Step 1:
> > > 
> > > * git send-email needs to include parent SHA1s and generally all the
> > >   information needed to perfectly recreate the commit when applied so
> > >   that all the SHA1s remain the same
> > > 
> > > * git am (or an alternative command) needs to recreate the commit
> > >   perfectly when applied, including applying it to the correct parent
> > > 
> > > Having these two will allow a perfect mapping between email and git;
> > > essentially email just becomes a transport for git. There are a lot of
> > > advantages to this, particularly that you have a stable way to refer to
> > > a patch or commit (despite it appearing on a mailing list), and there
> > > is no need for "changeset IDs" or whatever, since you can just use the
> > > git SHA1 which is unique, unambiguous, and stable.
> 
> I wonder if it'd be also possible to then embed gpg signatures over
> send-mail payloads so as they can be transparently transferred to the
> commit.

That's a crazy idea.  It would be nice if we could do that, I like it :)

greg k-h

Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc

[Pratyush Yadav]

On 14/10/19 12:18AM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Mon, 14 Oct 2019, Pratyush Yadav wrote:
> 
> > On 12/10/19 11:24PM, Johannes Schindelin wrote:
> > > Hi Pratyush,
> > >
> > > On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> > >
> > > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> > > >
> > > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > > > >   global HEAD PARENT MERGE_HEAD commit_type
> > > > >   global ui_index ui_workdir ui_comm
> > > > >   global rescan_active file_states
> > > > > - global repo_config
> > > > > + global repo_config _gitdir_cache
> > > > >
> > > > >   if {$rescan_active > 0 || ![lock_index read]} return
> > > > >
> > > > > + # Only re-prime gitdir cache on a full rescan
> > > > > + if {$after ne "ui_ready"} {
> > > >
> > > > What do you mean by a "full rescan"? I assume you use it as the
> > > > differentiator between `ui_do_rescan` (called when you hit F5 or choose
> > > > rescan from the menu) and `do_rescan` (called when you revert a line or
> > > > hunk), and a "full rescan" refers to `ui_do_rescan`.
> > > >
> > > > Well in that case, this check is incorrect. `do_rescan` passes only
> > > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> > > >
> > > > But either way, I'm not a big fan of this. This check makes assumptions
> > > > about the behaviour of its callers based on what they pass to $after.
> > > > The way I see it, $after should be a black box to `rescan`, and it
> > > > should make absolutely no assumptions about it.
> > > >
> > > > Doing it this way is really brittle, and would break as soon as someone
> > > > changes the behaviour of `ui_do_rescan`. If someone in the future passes
> > > > a different value in $after, this would stop working as intended and
> > > > would not refresh the cached list on a rescan.
> > > >
> > > > So, I think a better place for this if statement would be in
> > > > `ui_do_rescan`. This would mean adding a new function that does this.
> > > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not
> > > > to), we can get away with just something like:
> > > >
> > > >   proc ui_do_rescan {} {
> > > > rescan {prime_gitdir_cache; ui_ready}
> > > >   }
> > > >
> > > > Though since `prime_gitdir_cache` does not really depend on the rescan
> > > > being finished, something like this would also work fine:
> > > >
> > > >   proc ui_do_rescan {} {
> > > > rescan ui_ready
> > > > prime_gitdir_cache
> > > >   }
> > >
> > > That was my first attempt. However, there is a very important piece of
> > > code that is even still quoted above: that `if {$rescan_active > 0 ||
> > > ![lock_index read]} return` part.
> > >
> > > I do _not_ want to interfere with an actively-going-on rescan. If there
> > > is an active one, I don't want to re-prime the `_gitdir` cache.
> >
> > Good catch! In that case I suppose refreshing the cache in $after would
> > be the way to go (IOW, the former of my two suggestions). Anything
> > $after won't get executed if we return early from that check.
> 
> The obvious problem with that solution is that the `_gitdir` is reset
> _after_ the rescan. In my solution, it is reset _before_, as I have no
> idea how often the `_gitdir` values are used during a rescan (and if
> none of they were, I would like to be very cautious to prepare for a
> future where any of those `_gitdir` values _are_ used during a rescan).

_gitdir values are used quite often during a rescan, so yes, this won't 
work.
 
> So I am afraid that I find way too serious problems with both of your
> proposed alternatives.

One alternative I can see right now is adding another optional parameter 
to `rescan` that controls whether we refresh the gitdir cache or not. 
That parameter would default to 0/false. I'm not the biggest fan of 
something like this, but it might be the easiest way to do it given the 
constraints.

I also thought about trying to acquire the index lock in 
`prime_gitdir_cache`, but that could create a race on the lock between 
`prime_gitdir_cache` and `rescan`.

If you have any better ideas, I'm all ears, but otherwise, I maybe our 
best bet is to just go with adding an optional parameter to `rescan`.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

[Pratyush Yadav]

On 17/10/19 07:08AM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav  wrote:
> > I mentioned this earlier, and I'll mention this again: I'm not sure
> > whether this feature would be a good thing for the larger population. So
> > this _might_ not end up being accepted depending on how people react to
> > the proposal. I thought I'd let you know to avoid any nasty surprises
> > later.
> 
> Could you please elaborate on why you think the feature might be
> undesired? Why would users not want a staged file to be selected
> automatically?

I have similar arguments to what Philip said in the GitHub link you 
sent. I think software should do what the user tells it to do, and 
should not try to "second guess" them. This second guessing can get 
annoying.

So if I select the commit message buffer, I told the software to select 
it by clicking it. I did not tell it to switch my diff view. This switch 
of view can prove to be disorienting/confusing to a user because they 
did not select any diff. They selected a text box.

At the very least, I think we should have a config option, and it should 
be turned off by default. That said, I'd certainly like to hear what 
other people think on this topic.
 
> FWIW I've also got 2 comments on this in GH[1].
> 
> [1] https://github.com/git-for-windows/git/issues/2341

-- 
Regards,
Pratyush Yadav

Re: [PATCH 5/6] completion: list existing working trees for 'git worktree' subcommands

[Eric Sunshine]

On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor  wrote:
> Complete the paths of existing working trees for 'git worktree's
> 'move', 'remove', 'lock', and 'unlock' subcommands.
> [...]
> Arguably 'git worktree unlock ' should only complete locked
> working trees, but 'git worktree list --porcelain' doesn't indicate
> which working trees are locked.  So for now it will complete the paths
> of all existing working trees, including non-locked ones as well.

It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
to indicate whether a worktree is locked, prunable, etc. Looking at
the implementation of builtin/worktree.c:show_worktree_porcelain(), it
should be easy enough to add. (Adding it for the non-porcelain case
would perhaps require more thinking and design since we might want a
--verbose option to trigger the extra information.)

[1]: 
https://public-inbox.org/git/capig+cttrv2c7jlu1dr4+n8xo+7yq+deiwlda835wbgd6fh...@mail.gmail.com/

> Signed-off-by: SZEDER Gábor 
> ---
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> @@ -2981,10 +2981,21 @@ _git_whatchanged ()
> +__git_complete_worktree_paths ()
> +{
> +   local IFS=$'\n'
> +   __gitcomp_nl "$(git worktree list --porcelain |
> +   sed -n -e '2,$ s/^worktree //p')"
> +}

I know that the commit message talks about it, but it might deserve an
in-code comment here (or a function-level comment) explaining why the
first line of "git worktree list --porcelain" output is thrown away
since it's not necessarily obvious to the casual reader.

Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists

[Junio C Hamano]

Denton Liu  writes:

> There are many += lists in the Makefile and, over time, they have gotten
> slightly out of order, alphabetically. Alphabetically sort all += lists
> to bring them back in order.
> ...

Hmm.  I like the general thrust, but ...

>  LIB_OBJS += combine-diff.o
> -LIB_OBJS += commit.o
>  LIB_OBJS += commit-graph.o
>  LIB_OBJS += commit-reach.o
> +LIB_OBJS += commit.o

... I do not particularly see this change (there may be similar
ones) desirable.  I'd find it it be much more natural to sort
"commit-anything" after "commit", and that is true with or without
the common extension ".o" added to these entries.

In short, flipping these entries because '.' sorts later than '-' is
making the result look "less sorted", at least to me.

Thanks.

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[Eric Sunshine]

On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor  wrote:
> When using the __git_find_on_cmdline() helper function so far we've
> only been interested in which one of a set of words appear on the
> command line.  To complete options for some of 'git worktree's
> subcommands in the following patches we'll need not only that, but the
> index of that word on the command line as well.
>
> Extend __git_find_on_cmdline() to optionally show the index of the
> found word on the command line (IOW in the $words array) when the
> '--show-idx' option is given.
>
> Signed-off-by: SZEDER Gábor 
> ---
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> @@ -1069,18 +1069,32 @@ __git_aliased_command ()
>  # Check whether one of the given words is present on the command line,
>  # and print the first word found.
> +#
> +# Usage: __git_find_on_cmdline []... ""
> +# --show-idx: Optionally show the index of the found word in the $words 
> array.
>  __git_find_on_cmdline ()
>  {
> -   local word c=1
> +   local word c=1 show_idx
> +
> +   while test $# -gt 1; do
> +   case "$1" in
> +   --show-idx) show_idx=y ;;
> +   *)  return 1 ;;

Should this emit an error message to aid a person debugging a test
which fails on a call to __git_find_on_cmdline()? For instance:

echo "unrecognized option/argument: $1" >&2
return 1
;;

or something...

> +   esac
> +   shift
> +   done
> local wordlist="$1"
>
> while [ $c -lt $cword ]; do
> for word in $wordlist; do
> if [ "$word" = "${words[c]}" ]; then
> -   echo "$word"
> +   if [ -n "$show_idx" ]; then
> +   echo "$c $word"
> +   else
> +   echo "$word"
> +   fi
> return
> fi
> done

Re: [PATCH 2/2] Make "git branch -d" prune missing worktrees automatically.

[Eric Sunshine]

On Thu, Oct 17, 2019 at 12:28 PM Peter Jones  wrote:
> Currently, if you do:
>
> $ git branch zonk origin/master
> $ git worktree add zonk zonk
> $ rm -rf zonk
> $ git branch -d zonk
>
> You get the following error:
>
> $ git branch -d zonk
> error: Cannot delete branch 'zonk' checked out at 
> '/home/pjones/devel/kernel.org/git/zonk'
>
> It isn't meaningfully checked out, the repo's data is just stale and no
> longer reflects reality.

Echoing SEZDER's comment on patch 1/2, this behavior is an intentional
design choice and safety feature of the worktree implementation since
worktrees may exist on removable media or remote filesystems which
might not always be mounted; hence, the presence of commands "git
worktree prune" and "git worktree remove".

A couple comment regarding this patch...

> Signed-off-by: Peter Jones 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -133,6 +133,20 @@ static int prune_worktree(const char *id, struct strbuf 
> *reason)
> +int prune_worktree_if_missing(const struct worktree *wt)
> +{
> +   struct strbuf reason = STRBUF_INIT;
> +
> +   if (access(wt->path, F_OK) >= 0 ||
> +   (errno != ENOENT && errno == ENOTDIR)) {
> +   errno = EEXIST;
> +   return -1;
> +   }
> +
> +   strbuf_addf(&reason, _("Removing worktrees/%s: worktree directory is 
> not present"), wt->id);
> +   return prune_worktree(wt->id, &reason);
> +}

"git worktree" tries to clean up after itself as much as possible. For
instance, it is careful to remove the .git/worktrees directory when
the last worktree itself is removed (or pruned). So, the caller of
this function would also want to call delete_worktrees_dir_if_empty()
to follow suit.

> diff --git a/worktree.h b/worktree.h
> @@ -132,4 +132,10 @@ void strbuf_worktree_ref(const struct worktree *wt,
> +/*
> + * Prune a worktree if it is no longer present at the checked out location.
> + * Returns < 0 if the checkout is there or if pruning fails.
> + */
> +int prune_worktree_if_missing(const struct worktree *wt);

It's rather ugly that this function is declared in top-level
worktree.h whereas the actual implementation is in builtin/worktree.c.
I'd expect to see a preparatory patch which moves prune_worktree()
(and probably delete_worktrees_dir_if_empty()) to top-level
worktree.c.

These minor implementation comments aside, before considering this
patch series, it would be nice to see a compelling argument as to why
this change of behavior, which undercuts a deliberate design decision,
is really desirable.

Re: [PATCH 1/2] Make die_if_checked_out() ignore missing worktree checkouts.

[SZEDER Gábor]

On Thu, Oct 17, 2019 at 12:28:25PM -0400, Peter Jones wrote:
> Currently if you do, for example:
> 
> $ git worktree add path foo
> 
> And "foo" has already been checked out at some other path, but the user
> has removed it without pruning, you'll get an error that the branch is
> already checked out.  It isn't meaningfully checked out, the repo's
> data is just stale and no longer reflects reality.
> 
> This makes it so that if nothing is present where a worktree is
> supposedly checked out, we ignore that the worktree exists, and let it
> get cleaned up the next time worktrees are pruned.
> 
> (I would prune it instead, but prune isn't available from libgit
> currently.)
> 
> Signed-off-by: Peter Jones 
> ---
>  branch.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/branch.c b/branch.c
> index 579494738a7..60322ded953 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -360,6 +360,9 @@ void die_if_checked_out(const char *branch, int 
> ignore_current_worktree)
>   wt = find_shared_symref("HEAD", branch);
>   if (!wt || (ignore_current_worktree && wt->is_current))
>   return;
> + if (access(wt->path, F_OK) < 0 &&
> + (errno == ENOENT || errno == ENOTDIR))
> + return;

I think this check is insuffient: even if the directory of the working
tree is not present, the working tree might still exist, and should
not be ignored (or deleted/pruned in the second patch).

See the description of 'git worktree lock' for details.

>   skip_prefix(branch, "refs/heads/", &branch);
>   die(_("'%s' is already checked out at '%s'"),
>   branch, wt->path);
> -- 
> 2.23.0
> 

Re: email as a bona fide git transport

[Steven Rostedt]

On Thu, 17 Oct 2019 16:01:33 +0200
Vegard Nossum  wrote:

> In your example, couldn't Darrick simply base his xfs work on the latest
> xfs branch that was pulled by Linus? That should be up to date with all
> things xfs without having any of the things that made Linus's tree not
> work for him.

Sure, but why?

I thought this whole exercise is to make the process easier. This seems
to be making it more complex. Now we are going to be demanding
submitters to be basing their work on a specific (older) commit.

I always tell people that submit to me, to base off of one of Linus's
latest tags. That's what I do.

-- Steve

Re: email as a bona fide git transport

[Theodore Y. Ts'o]

On Thu, Oct 17, 2019 at 04:01:33PM +0200, Vegard Nossum wrote:
> 
> In your example, couldn't Darrick simply base his xfs work on the latest
> xfs branch that was pulled by Linus? That should be up to date with all
> things xfs without having any of the things that made Linus's tree not
> work for him.

Sure, but sometimes there are changes in subsystems which the file
system depends upon that were also merged by Linus.  So for example,
the ext4 branch might be based on v5.3-rc4, and gets pulled after v5.3
is released, along with a huge number of other subsystem trees, so the
delta between v5.3 and v5.3-rc1 is ***huge***.

So while I could base my development on my previous ext4.git branch
(based off of v5.3-rc4), at *some* point I need to be able to sync up
with upstream.  And the usual way to do this is to start a new
development branch based on (for example) v5.4-rc2, or in some cases
v5.4-rc4.

We could keep the development branch on based off of v5.3-rc4, and
wait until things stablize, and *then* merge in v5.4-rcX, when
v5.4-rcX is finally stable, but that makes for a more complex merge,
and so it means that things like "git log origin..master" don't really
work any more.  So the preferred development practice is very much

   rc2 o --> patch 1 --> patch 2 --> ... --> patch N 
(origin) (master)

Where the "master" branch gets merged into the rewinding "dev" branch
(which works much like git's pu branch), and where the "master" branch
is what Linus will merge at the next merge window.

> Otherwise, you could apply the stabilisation patches and then do your
> final testing in a branch that merges that with your patchset, like so:
> 
>rc1 o -> fixup A --> fixup B >o merge (tested)
> (base)  \   /
>  \ /
>   ---> patch 001 --> patch 002 -->o patchset (submitted)

I cloud do that, but remember that the checked out kernel tree is
about a gigabyte (this assumes using git clone --shared, so it doesn't
include the git pack files, and this is source only; the object files
are another 2.6 GB).  I could keep separate checked out trees, and
separate build trees, but that burns a lot of disk/SSD space.  Or I
could switch back and forth by using "git checkout" between the
development branch and the branch with the stablization patches, but
then I'm constantly having to rebuild the object files, and ccache
only helps so much.

So it's much simpler to put the fixup patches at the on top of the
origin, and then mail them out without having to play git branch
rebasing gymnastics.  When the patch series is finally ready to roll,
then the maintainer will apply the patch series on a clean branch,
since hopefully by then -rc3 or -rc4 is finally stable enough to use
as an origin point.

So the idea is that developer might be sending out revisions of their
patches on top of -rc1 plus fixup patches, but then the final version
of the patch series, after a few rounds of review, gets applied on top
of -rc3 or -rc4.

> I think the more difficult problem to solve might be how to ensure that
> the base commit is actually public/reachable when this is the intention.
> A bot watching the mailing list could always respond with a "Hey, I
> don't have that, could you rebase the series or push it somewhere?". But
> it would be even better if git could tell you when you're about to
> submit a patch. Maybe something like:
> 
>   git send-email --ensure-reachable-from [remote] rev^^..
> 
> In the worst case, I guess the base commit will just not be available --
> the email will still have a sha1 on it, though, and which might still be
> usable as an identifier for the patch/patchset. If not, it's still not
> worse than the current workflow (which would still work).

... or what we can do is allow the developer to specify the intended
base --- e.g., -rc1, even though his patchset was against "-rc1 plus fixups".

 - Ted

Re: git smart http + apache mod_auth_openidc

[Ralph Ewig]

Quick follow up question: can the git client pass 
a token read from a cookie with a request? That 
would enable users to sign-in via a browser, store 
the cookie, and then use that as the access token 
to authenticate a git request.


On 10/16/2019 11:03 PM, Jeff King wrote:
> On Thu, Oct 17, 2019 at 03:00:58AM +, Ralph Ewig wrote:
>
>> Thanks for the reply. I was hoping the Git GUI
>> might be able to handle the OpenID authentication
>> flow, but it makes sense that it would be
>> inconsistent with other git clients.
> I don't think we'd ever do the full flow, but it might be that it's
> possible to use a token generated by the flow in a separate browser
> session. This is easy to do if you can provide the token to the server
> in place of a password. E.g., if you use 2FA on GitHub, you can generate
> a limited token that allows you to connect for Git operations.
>
> You can also use time-limited tokens and feed them from a custom
> credential helper, which does the full authentication flow from a web
> browser. I think Google does (or did) have such a thing internally, but
> I don't think any part of it was made public (it was discussed on the
> list a couple times in the early days of credential helpers).
>
> That would still have to end up with a token "password" to send over
> basic auth, I think. There was some discussion a while back of letting
> credential helpers pass back content for HTTP "Authorization" headers,
> but I don't think anything was ever merged.
>
    C:\Users\void>git clone --progress -v
 "https://git.xxx.xxx/WebApps.git";
    Cloning into 'WebApps'...
    fatal: unable to update url base from
 redirection:
  asked for:
 https://git.xxx.xxx/WebApps.git/info/refs?service=git-upload-pack
   redirect:
 https://login.microsoftonline.com/xxx/oauth2/authorize?response_type=code&scope=openid&client_id=xxx&state=xxx&redirect_uri=https%3A%2F%2Fgit.xxx.xxx%2Fredirect&nonce=xxx
> You're running into one other complexity here, which is that we don't
> allow cross-server redirects during the initial conversation. So even if
> you did have an auth flow where we could somehow provide all the
> information via Git, we'd still be unhappy about bouncing between
> multiple servers.
>
> Mostly that's there to avoid leaking credentials. We only have one
> notion of "the username and password we're using" for a given fetch, so
> we want to avoid leaking it if we're redirected to another server. But
> obviously Git _could_ be smarter there, if this was the only blocker
> remaining (but from my understanding of OpenID, it's not).
>
> -Peff

Re: git smart http + apache mod_auth_openidc

[Ralph Ewig]

Interesting ... I have not looked at access tokens 
before, but did find some documentation online 
that describes how Azure AD Jason Web Tokens can 
be used to authenticate a headless API 
(https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens), 


This seems to be a fit in this scenario, but  is 
admittedly a bit deeper into the weeds than I'm 
familiar with. I'll keep digging in to see if I 
can find a solution along that route. Really 
appreciate the pointer!

Ralph


On 10/16/2019 11:03 PM, Jeff King wrote:
> On Thu, Oct 17, 2019 at 03:00:58AM +, Ralph Ewig wrote:
>
>> Thanks for the reply. I was hoping the Git GUI
>> might be able to handle the OpenID authentication
>> flow, but it makes sense that it would be
>> inconsistent with other git clients.
> I don't think we'd ever do the full flow, but it might be that it's
> possible to use a token generated by the flow in a separate browser
> session. This is easy to do if you can provide the token to the server
> in place of a password. E.g., if you use 2FA on GitHub, you can generate
> a limited token that allows you to connect for Git operations.
>
> You can also use time-limited tokens and feed them from a custom
> credential helper, which does the full authentication flow from a web
> browser. I think Google does (or did) have such a thing internally, but
> I don't think any part of it was made public (it was discussed on the
> list a couple times in the early days of credential helpers).
>
> That would still have to end up with a token "password" to send over
> basic auth, I think. There was some discussion a while back of letting
> credential helpers pass back content for HTTP "Authorization" headers,
> but I don't think anything was ever merged.
>
    C:\Users\void>git clone --progress -v
 "https://git.xxx.xxx/WebApps.git";
    Cloning into 'WebApps'...
    fatal: unable to update url base from
 redirection:
  asked for:
 https://git.xxx.xxx/WebApps.git/info/refs?service=git-upload-pack
   redirect:
 https://login.microsoftonline.com/xxx/oauth2/authorize?response_type=code&scope=openid&client_id=xxx&state=xxx&redirect_uri=https%3A%2F%2Fgit.xxx.xxx%2Fredirect&nonce=xxx
> You're running into one other complexity here, which is that we don't
> allow cross-server redirects during the initial conversation. So even if
> you did have an auth flow where we could somehow provide all the
> information via Git, we'd still be unhappy about bouncing between
> multiple servers.
>
> Mostly that's there to avoid leaking credentials. We only have one
> notion of "the username and password we're using" for a given fetch, so
> we want to avoid leaking it if we're redirected to another server. But
> obviously Git _could_ be smarter there, if this was the only blocker
> remaining (but from my understanding of OpenID, it's not).
>
> -Peff

Re: email as a bona fide git transport

[Vegard Nossum]

On 10/17/19 3:11 PM, Theodore Y. Ts'o wrote:

On Thu, Oct 17, 2019 at 02:23:58PM +0200, Vegard Nossum wrote:

Of course, this relies strongly on actually having (correct) sha1
references to previous versions inside the changelog. In my original
idea, this reference would only appear inside the merge commit that
binds the patchset together to minimise churn, although maybe it is
feasible to also append it to each patch -- in that case, the "patchset"
command from my first email is not sufficient to create a new version of
a patchset.


This also relies on the base of the commit actually being a public
SHA1.  Sometimes developers will cherry-pick in a patch that they need
so that the kernel will actually *boot* (or otherwise fix problems
that have been fixed in other subsystems, but not yet landed in -rc2
or -rc3).

Of course, we could tell people that they should always create their
patches off of the last stable version (but then there may have been
changes pulled in via the last merge window that makes their patch not
apply), or they could be told to develop against -rc2 or -rc3, and
then cherry pick the required fix-up patches on top of -rc2 and -rc3,
but then they have to do a lot more rebuilding.

So there are no perfect solutions here, and while in the ideal world,
-rc2 and -rc3 should be perfectly stable enough for developers so that
they never need to manually patch in stablization patches, we need to
live in the real world.  I believe that Darrick told me that in the
previous development cycle, he had to wait until -rc4 before the tree
was stable enough for him to start building xfs patches on top
mainline.

(This is also true for this development cycle if you enable
CONFIG_KMEMLEAK, although fortunately, the workaround that worked for
me was to just CONFIG_KMEMLEAK --- although of course, if I do have to
run a KMEMLEAK test run, I'll need to cherry-pick the fix which landed
this week on top of the ext4 git tree.)

What this all might mean is that sometimes it will make sense to allow
the user to override the base commit so such stablization patches can
be elided.  Of course, we could force the user to create a separate
branch and rebase, but can be quite painful and slow --- and they
won't be able to test the rebased branch anyway, unless we then want
to tell them to cherry pick the stablization patches on top, and then
remove them before running "git send-email".


Good points.

I suspect that you should almost always be able to find a good base
revision to build and test your changes on.

In your example, couldn't Darrick simply base his xfs work on the latest
xfs branch that was pulled by Linus? That should be up to date with all
things xfs without having any of the things that made Linus's tree not
work for him.

Otherwise, you could apply the stabilisation patches and then do your
final testing in a branch that merges that with your patchset, like so:

   rc1 o -> fixup A --> fixup B >o merge (tested)
(base)  \   /
 \ /
  ---> patch 001 --> patch 002 -->o patchset (submitted)

It does not seem too hard to me, and it should be pretty safe from a
test-what-you-ship point of view assuming the fixups and your patches
really are independent.

I think the more difficult problem to solve might be how to ensure that
the base commit is actually public/reachable when this is the intention.
A bot watching the mailing list could always respond with a "Hey, I
don't have that, could you rebase the series or push it somewhere?". But
it would be even better if git could tell you when you're about to
submit a patch. Maybe something like:

  git send-email --ensure-reachable-from [remote] rev^^..

In the worst case, I guess the base commit will just not be available --
the email will still have a sha1 on it, though, and which might still be
usable as an identifier for the patch/patchset. If not, it's still not
worse than the current workflow (which would still work).


Vegard

Re: Raise your hand to Ack jk/code-of-conduct if your Ack fell thru cracks

[Philip Oakley]

Hi all,

On 11/10/2019 06:58, Jeff King wrote:

I snipped your concerns with some of the language. I do agree with you
that a lot of is open to interpretation. But I also think it's
impossible to get it 100% airtight. My feeling was that it was a good
idea to go with some existing, well-established text, even if it has
some wiggle room. And then rely on the existing community and especially
the people listed above to do that interpretation.

So...


Just pointing out some concerns of mine.  No ack from me
(but it's not a NACK, either).  I'm pretty ambivalent...

For me it is obviously an ack, but I wanted to make clear that I think
your concerns (and those of others who spoke up, like René and Gábor)
are certainly_valid_. I just think that adopting this CoC is, while not
perfect, the least-bad option.

I'd also say that we might consider living with it for a while (6
months? a year?) and seeing if people have an interest in revising it
after that point based on experience.

 I also didn't positively ack the CoC (code of conduct).

I'm not sure it addresses the broader _underlying_ issues that may need 
to be addressed that are behind the pressure for CoCs. I'd also 
commented [1] on the git-for-windows CoC partly because the CoC didn't 
positively address the need for tolerance.


These CoCs are essentially defensive, rather than forward looking. In 
essence they say:


We are a welcoming and inspiring community, open to anyone and 
everyone(all 2^16 variants).


We list various egregious behaviours that are unwanted and hence 
intolerable.


We list responses to such intolerable behaviour.

However we (in the CoC document) don't really address what we may need 
to do to extend the community to the broader many.


Part of the wider problem is we often don't appreciate our pre-existing 
organisational biases (e.g.[2, 3]) that we fit into within a community. 
For example the implicit gender bias toward independent sole author 
contributions[4], rather than the inclusiveness of co-authorship as a norm.


While following peff's "interpretation" document link [5], I did see, in 
the wider kernel document, that it does have a "Co-developed-by:" option 
[6] but then requires a secondary "Signed-off-by:", thus making those 
who co-author do extra work, which shouldn't be required.


Thus, while the CoC is good, for clarifying the egregious behaviour 
issues, it doesn't really address the wider 'Diversity and Equality' 
*expectations* within the community.


Philip

[1] https://github.com/git-for-windows/git/pull/661#issuecomment-186846113
[2] "institutional racism" 
https://en.wikipedia.org/wiki/Institutional_racism

[3] "institutional sexism" ... no Wikipedia article?
[4] 
https://www.computing.co.uk/ctg/sponsored/3082288/want-to-increase-diversity-it-starts-with-the-job-ad
[5] 
https://www.kernel.org/doc/html/latest/process/code-of-conduct-interpretation.html

[6] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Re: email as a bona fide git transport

[Vegard Nossum]

On 10/17/19 5:17 AM, Junio C Hamano wrote:

Vegard Nossum  writes:


Step 1:

* git send-email needs to include parent SHA1s and generally all the
   information needed to perfectly recreate the commit when applied so
   that all the SHA1s remain the same

* git am (or an alternative command) needs to recreate the commit
   perfectly when applied, including applying it to the correct parent


You can record and convey the commit object name a series is meant
to be applied on top already, and it in general is a good way to
give a wider context in order to explain and justify the series.

On the other hand, "all the information needed to recreate..." is
not very useful.  If you want the commit object to be exactly what
you want to see at the tip of the end result, you are better off
asking your upstream to pull.  Using e-mail for that makes you and
project participants give up a lot of benefits the workflow based on
e-mail gives you, the biggest of which is the ease of giving
suggestions for improvements.  Once you insist "perfectly recreate
the commit", you are not willing to take any input from the
sidelines---worse yet, you are even dictating when the upstream
runs "git am" to turn them into commits, and do so without reading
the patches (there is no point reviewing as the person who runs "git
am" is not even allowed to fix typo or make obvious fixes to the
code, which will fail to perfectly recreate the commit).

In short, one should resist temptation to bring up "perfect
reproduction" when one talks about e-mail workflow.


Please see what I wrote to Pratyush Yadav here:

https://public-inbox.org/git/a1c33600-14e6-be37-c026-8d8b8e4ba...@oracle.com/

TL;DR: the goal is not necessarily for maintainers to be able to merge
the patchset with the same SHA1 that the submitter had, but for the
patchset to have a definite SHA1 that lives in git, and which can be
used by all the participants -- submitter, reviewers, bots (including
potentially testing/CI infrastructure), and maintainers.

I am definitely not proposing to get rid of the email workflow -- on the
contrary, this it the workflow I want to preserve! :-) The "workflows"
mailing list was created for the purpose of discussing this topic (in
the context of Linux kernel development) and right now there are many
proposals that either completely cut out email or reduce it to something
like pull requests. My proposal keeps almost everything the same, except
for a few lines of extra metadata before the actual diff.

(I will answer the rest of your email separately.)


Vegard

Re: email as a bona fide git transport

[Theodore Y. Ts'o]

On Thu, Oct 17, 2019 at 02:23:58PM +0200, Vegard Nossum wrote:
> Of course, this relies strongly on actually having (correct) sha1
> references to previous versions inside the changelog. In my original
> idea, this reference would only appear inside the merge commit that
> binds the patchset together to minimise churn, although maybe it is
> feasible to also append it to each patch -- in that case, the "patchset"
> command from my first email is not sufficient to create a new version of
> a patchset.

This also relies on the base of the commit actually being a public
SHA1.  Sometimes developers will cherry-pick in a patch that they need
so that the kernel will actually *boot* (or otherwise fix problems
that have been fixed in other subsystems, but not yet landed in -rc2
or -rc3).

Of course, we could tell people that they should always create their
patches off of the last stable version (but then there may have been
changes pulled in via the last merge window that makes their patch not
apply), or they could be told to develop against -rc2 or -rc3, and
then cherry pick the required fix-up patches on top of -rc2 and -rc3,
but then they have to do a lot more rebuilding.

So there are no perfect solutions here, and while in the ideal world,
-rc2 and -rc3 should be perfectly stable enough for developers so that
they never need to manually patch in stablization patches, we need to
live in the real world.  I believe that Darrick told me that in the
previous development cycle, he had to wait until -rc4 before the tree
was stable enough for him to start building xfs patches on top
mainline.

(This is also true for this development cycle if you enable
CONFIG_KMEMLEAK, although fortunately, the workaround that worked for
me was to just CONFIG_KMEMLEAK --- although of course, if I do have to
run a KMEMLEAK test run, I'll need to cherry-pick the fix which landed
this week on top of the ext4 git tree.)

What this all might mean is that sometimes it will make sense to allow
the user to override the base commit so such stablization patches can
be elided.  Of course, we could force the user to create a separate
branch and rebase, but can be quite painful and slow --- and they
won't be able to test the rebased branch anyway, unless we then want
to tell them to cherry pick the stablization patches on top, and then
remove them before running "git send-email".

- Ted

Re: email as a bona fide git transport

[Vegard Nossum]

On 10/16/19 10:57 PM, Jonathan Nieder wrote:

Hi,

A few small points.

Vegard Nossum wrote:


* git am (or an alternative command) needs to recreate the commit
   perfectly when applied, including applying it to the correct parent


Interesting.  "git format-patch" has a --base option to do some of
what you're looking for, for the sake of snowpatch
.  Though it's not exactly the
same thing you mean.


Yes, --base is great for importing email patches into git, but does not
allow resulting commits to have the same sha1 (because it doesn't have
the complete author/committer information, mainly), so it doesn't do
enough for what I want with this proposal.


We also discussed sending merge commits by mail recently in the
virtual git committer summit[1].

Of course, the devil is in the details.  It's straightforward to use
"git bundle" to use mail as a Git transport today, but presumably you
also want the ability to perform reviews along the way and that's not
so easy with a binary format.  Do you have more details on what you'd
want the format to look like, particularly for merge commits?


Yes, one of the goals is to continue to use git-send-email and be able
to view and review patches on a mailing list with minimal intrusion to
existing processes.

I did not envision supporting merge commits where the resulting tree is
different from all of its parents, which means any merge commits run
through 'git-format-patch --complete' would just have multiple "parent"
lines and no diff where the diff would normally be.


is no need for "changeset IDs" or whatever, since you can just use the
git SHA1 which is unique, unambiguous, and stable.


In [2] the hope was for some identifier that is preserved by "git
rebase" and "git commit --amend" (so that you can track the evolution
of a change as the author improves it in response to reviews).  Is
that the conversation you're alluding to?


Yes.

(Not the specific email/thread you linked, but yes, this is the general
conversation about having stable identifiers I'm referring to.)



[...]

Disadvantages:

- requires patching git


That's not a disadvantage.  It means get to work with the Git project,
which is a welcoming bunch of people, working on userspace (seeing how
the other half lives), and improving the lives of everyone using Git.


True! :-)


Date: Sat, 5 Oct 2019 16:15:59 +0200
Subject: [PATCH 1/3] format-patch: add --complete

Include the raw commit data between the changelog and the diffstat.


Oh!  I had missed this on first reading because it was in an
attachment.

I have mixed feelings.  Can you say a bit more about the advantages
and disadvantages relative to sending a git bundle?  What happens if a
mail client or a box along the way mangles whitespace in the commit
message?


Yes, as we both said above: git bundle is not human readable and does
not lend itself to reviews in mail clients or on mailing lists.

Any kind of mangling is a serious concern, and my thoughts are:

 - The _diff_ itself should already be safe from mangling. If this were
not the case, then sending patches by email would be completely unsafe
and would need to be fixed somehow anyway.

 - I think I remember seeing something in either 'git am' or 'git
mailinfo' about format=flowed apparently allowing whitespace to
disappear from the line endings.

 - Isolated problems like that could be preemptively fixed (or at least
warned about) on the submitter side by e.g. warning about potential
issues when committing or running format-patch/send-email)

 - If some mail software is susceptible to mangling patches, then I
think it would great to have a mechanism for detecting that -- which
"git-format-patch --complete" would be! :-)

 - It is true that the commit message itself (and perhaps particularly
people's names) is especially vulnerable to mangling and/or reencoding
(I noticed that 'git am' seems to convert to utf8 before committing). I
honestly don't know if this would be a problem in practice, as I don't
know too much about mail software and encodings. If most people use
format-patch/send-email to send patches, then maybe it's not actually a
problem because everything is either kept as utf-8 to start with or
encoded/decoded consistently across the git userbase?

 - I was thinking of hex-encoding the raw bytes of the author and
committer lines of the extra commit metadata in the final email to keep
everything as ASCII and avoid character set conversions. It is true that
this does not stop mangling of the changelog...

 - I suspect that if mangling was a problem in practice, then we would
have seen it already and the solution to it is not specific to what I'm
proposing here.

I'd love for somebody who is more knowledgeable about email and encoding
to chime in here. It is definitely one of the biggest potential problems
and it would be good to approach it in a way that doesn't require lots
of red tape after it has already been implemented.


Vegard



Happy

Re: [PATCH v3 08/13] graph: tidy up display of left-skewed merges

[Derrick Stolee]

On 10/16/2019 12:00 AM, Junio C Hamano wrote:
> "James Coglan via GitGitGadget"  writes:
> 
>> This effect is applied to both "normal" two-parent merges, and to
>> octopus merges. It also reduces the vertical space needed for pre-commit
>> lines, as the merge occupies one less column than usual.
>>
>> Before: After:
>>
>> | * | *
>> | |\| |\
>> | | \   | * \
>> | |  \  |/|\ \
>> | *-. \
>> | |\ \ \
> 
> Looking at these drawings reminded me of a tangent that is brought
> up from time to time.  We do not do great job when showing multiple
> roots.
> 
> If you have a history like this:
> 
>   A---D---E
>  /
> B---C
> 
> drawing the graph _with_ the merge gives a reasonable representation
> of the entire topology.
> 
> * 46f67dd E
> *   6f89516 D
> |\  
> | * e6277a9 C
> | * 13ae9b2 B
> * afee005 A
> 
> But if you start drawing from parents of D (excluding D), you'd get
> this:
> 
> * e6277a9 C
> * 13ae9b2 B
> * afee005 A

I hit this very situation recently when I was experimenting with
'git fast-import' and accidentally created many parallel, independent
histories. Running "git log --graph --all --simplify-by-decoration"
made it look like all the refs were in a line, but they were not.
(The one way I knew something was up: the base commits also appeared
without a decoration. That was the only clue that the histories did
not continue in a line.)

> 
> and the fact that B and A do not share parent-child relationships is
> lost.  An easy way to show that would be to draw the bottom three
> lines of the full history output we saw earlier:
> 
> | * e6277a9 C
> | * 13ae9b2 B
> * afee005 A
> 
> either with or without the vertical bar to imply that A may have a
> child.
The natural extension of this would be multiple columns:

 | | | | | *
 | | | | *
 | | | *
 | | *
 | *
 *

This does not appear too cumbersome, and such a situation should
be rare.

> This is not something that has to be done as part of this series,
> but I am hoping that the internal simplification and code
> restructuring that is done by this series would make it easier to
> enhance the system to allow such an output.

I agree. An excellent idea for a follow-up.

Thanks,
-Stolee

Re: [PATCH v3 07/13] graph: example of graph output that can be simplified

[Derrick Stolee]

On 10/15/2019 7:47 PM, James Coglan via GitGitGadget wrote:
> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> new file mode 100755
> index 00..4582ba066a
> --- /dev/null
> +++ b/t/t4215-log-skewed-merges.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description='git log --graph of skewed merges'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'log --graph with merge fusing with its left and right 
> neighbors' '
> + cat >expect <<-\EOF &&
> + *   H
> + |\
> + | *   G
> + | |\
> + | | * F
> + | | |
> + | |  \
> + | *-. \   E
> + | |\ \ \
> + |/ / / /
> + | | | /
> + | | |/
> + | | * D
> + | * | C
> + | |/
> + * | B
> + |/
> + * A
> + EOF

Thanks for adding this test! I really think it helps show some
of your improvements later as this test is mutated.

-Stolee

> +
> + git checkout --orphan _p &&
> + test_commit A &&
> + test_commit B &&
> + git checkout -b _q @^ && test_commit C &&
> + git checkout -b _r @^ && test_commit D &&
> + git checkout _p && git merge --no-ff _q _r -m E &&
> + git checkout _r && test_commit F &&
> + git checkout _p && git merge --no-ff _r -m G &&
> + git checkout @^^ && git merge --no-ff _p -m H &&
> +
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
> 

Re: email as a bona fide git transport

[Vegard Nossum]

On 10/16/19 5:00 PM, Pratyush Yadav wrote:

On 16/10/19 12:22PM, Vegard Nossum wrote:
Just to play the devil's advocate, even though I'm in favor of something
like this, I'll add in another disadvantage:

- The maintainer can't make small edits before pushing the changes out.

I do that every now and then for git-gui, and Junio does that sometimes
for Git. I don't know if the folks over at Linux do something like this,
but using '--exact' would mean that contributors would have to send a
re-roll for even minor changes. Its mostly an inconvenience instead of a
problem, but I thought I'd point it out.


I don't think this is a problem.

The point of 'git am --exact' is not for maintainers per se (although
they should use it if they don't have any manual changes to make), but
for the bot that keeps track of patchsets submitted via email.

The important part is that there is a git reference to the patchset that
was submitted in the patchset that was merged. You could see it as the
maintainer rolling a new version of the patchset locally and merging
that instead of merging what was submitted directly.

Of course, this relies strongly on actually having (correct) sha1
references to previous versions inside the changelog. In my original
idea, this reference would only appear inside the merge commit that
binds the patchset together to minimise churn, although maybe it is
feasible to also append it to each patch -- in that case, the "patchset"
command from my first email is not sufficient to create a new version of
a patchset.


One more question, not strictly related to your proposal: right now,
when I apply patches from contributors, I pass '-s' to 'am', so the
applied commit would have my sign-off. The way I see it, that sign-off
is supposed to signify that I have the right to push out the commit to
the "main" repo, just like the author's sign-off means that they have
the right to send me that commit.

Looking at git.git, I notice that Junio does the same. The new '--exact'
would be incompatible with '-s', correct (since the commit message has
changed, the SHA1 would also change)? So firstly, make sure you account
for something like that if you haven't already (I haven't found the time
to read your patches yet). Secondly, is it all right for the maintainer
to just not sign-off on the commits they push out?


In the Linux kernel at least, only the front-line maintainers add their
signoffs; higher-level maintainers take pull requests and don't add
their own sign-offs. Only Linus (and Greg, perhaps) has commit rights to
the main repository, but he only signs off on the patches that he either
writes himself or applies directly from email.

In any case, I don't think this is a concern because of what I wrote
above -- somebody who wants to add their signoff can do it by
essentially rolling a new version of the patchset that has the signoff,
but refers to the sha1 that was submitted to them by the patchset author
so that you can still find the original commits (without the signoffs)
and reviews/discussions.

I don't want to create extra work for maintainers, so I think any
solution should involve having the existing git tools/workflows do the
right thing automatically.

How about this? If 'git am' or 'git am -s' (without --exact) finds an
email patch where the exact commit metadata is present, it automatically
appends a line to the changelog saying where it was taken from:

Submitted-as: 

or

Applied-from: 

Although, again, this would modify the changelog of the patch itself
rather than just the changelog of the merge commit... Maybe this is
enough and we can have the "patchset" command also add references to
previous versions of each patch rather than the patchset as a whole.


Vegard

Re: [PATCH 0/1] ci: update caskroom/cask/perforce to new location

[Johannes Schindelin]

Hi Stolee,

On Tue, 15 Oct 2019, Derrick Stolee via GitGitGadget wrote:

> Running CI on Mac OS X in Azure Pipelines is currently broken due to a moved
> homebrew package.

It seems that the exact problem we tried to avoid by staying on caskroom
rears its ugly head: all of the macOS jobs are failing in Git for
Windows' builds (but not in GitGitGadget's, which is probably due to
differences in the "up to date"-ness used build agents).

I am investigating here:
https://github.com/git-for-windows/git/pull/2359

Ciao,
Dscho

>
> Thanks, -Stolee
>
> Johannes Schindelin (1):
>   ci(osx): use new location of the `perforce` cask
>
>  ci/install-dependencies.sh | 1 +
>  1 file changed, 1 insertion(+)
>
>
> base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6
> Published-As: 
> https://github.com/gitgitgadget/git/releases/tag/pr-400%2Fderrickstolee%2Fci-caskroom-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
> pr-400/derrickstolee/ci-caskroom-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/400
> --
> gitgitgadget
>
>

Re: [PATCH v2 1/3] doc: provide guidance on user.name format

[Junio C Hamano]

Jeff King  writes:

> I think this is a good distinction to draw, but...
>
>>  Documentation/git-commit-tree.txt | 6 ++
>>  1 file changed, 6 insertions(+)
>
> ...I was surprised to see it here, where I think most users wouldn't
> find it. Would it make sense in git-commit(1), or in the description of
> the user.name config?

Yeah, I had the same reaction as you had, both positive and negative
(eh, rather "surprised").

Re: [PATCH 1/1] builtin/blame.c: constants into bit shift format

[Junio C Hamano]

"Hariom Verma via GitGitGadget"  writes:

> -#define OUTPUT_SHOW_AGE_WITH_COLOR   04000
> +#define OUTPUT_ANNOTATE_COMPAT  (1<<0)
> +#define OUTPUT_LONG_OBJECT_NAME (1<<1)
> +#define OUTPUT_RAW_TIMESTAMP(1<<2)
> +#define OUTPUT_PORCELAIN(1<<3)
> +#define OUTPUT_SHOW_NAME(1<<4)
> +#define OUTPUT_SHOW_NUMBER  (1<<5)
> +#define OUTPUT_SHOW_SCORE   (1<<6)
> +#define OUTPUT_NO_AUTHOR(1<<7)
> +#define OUTPUT_SHOW_EMAIL   (1<<8)
> +#define OUTPUT_LINE_PORCELAIN   (1<<9)
> +#define OUTPUT_COLOR_LINE   (1<<10)
> +#define OUTPUT_SHOW_AGE_WITH_COLOR  (1<<11)

For these small shift counts it probably would not matter, but it
may be a good discipline to make sure they are treated as constants
of an unsigned type (i.e. write them as (1U<<0) etc.).  It probably
starts to matter when you reach 1<<31 if these are bits stuffed into
"unsigned int" on 32-bit arch.

One advantage of octal and hexadecimal notations have is that
0x8000 is automatically unsigned, IIRC, on such an archtecture.

Re: [PATCH 0/1] builtin/blame.c: bit field constants into bit shift format

[Junio C Hamano]

"Hariom Verma via GitGitGadget"  writes:

> we are looking at bitfield constants, and elsewhere in the Git source code,
> such cases are handled via bit shift operators rather than octal numbers,
> which also makes it easier to spot holes in the range (if, say, 1<<5 was
> missing, it is easier to spot it between 1<<4 and 1<<6 than it is to spot a
> missing 040 between a 020 and a 0100). Also, bit shifts lead to low-level
> optimizations because they require fewer calculations for the CPU. 

I think the last sentence is a nonsense for any decent compiler that
turns "1<<5" into 040 at compile time and treats it as literal
integer.  Luckily, it only appears here in the cover letter and does
not appear in the patch proper, so no need to resend the patch to
correct this ;-)

Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse

[Jeff King]

On Thu, Oct 17, 2019 at 04:03:00PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> Hmm, I am kind of surprised that the decoding side allowed such a
> >> padding.
> >
> > IIRC, the "padding" is just a sequence of 0-length-plus-continuation-bit
> > varint bytes. And for some reason it worked for the size but not for the
> > delta offset value.
> 
> I think the reason is because they use different varint definition.
> 
> The encoding used in builtin/pack-objects.c::write_no_reuse_object()
> is for offsets, and it came much later and with an improvement over
> the encoding used for delta size in diff-delta.c::create_delta().
> The more recent encoding does not allow padding (when I compare the
> decoders for these two encodings, I notice there is +1 for each
> 7-bit iteration; this essentially declares that a byte with "not the
> final byte" bit set with all other bits clear does not mean 0 but it
> means 1, which breaks the idea of padding to encode filler zero
> bits).

Yeah, that sounds right. I think the "old" one is actually the pack size
header in unpack_object_header_buffer(), not the delta size header.

At any rate, it seems like a terrible idea, so I'll be glad to see the
code dropped. :)

-Peff

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[Junio C Hamano]

SZEDER Gábor  writes:

> However, I'm not sure what the right path should be in the first
> place, given that each working tree has its own 'logs' directory, but
> only for HEAD's reflog, while everything else goes to the main working
> tree's 'logs' directory.

As all worktrees should share the same view of where 'master' (for
example) branch points at, what commit it was pointing at before,
etc., the reflogs should also be shared for refs.  The exception is
the HEAD where each worktree can point at its own commit (when detached)
or a branch (when not).

The above "should" does not mean "I know the code works that way so
if your git behaves differently your compiler is wrong"; it just
means "that's the logical conclusion of the basic design, and if
your git does not behave that way, we have a bug (or two)".

Thanks.

Re: [PATCH] remote-curl: pass on atomic capability to remote side

[Junio C Hamano]

"brian m. carlson"  writes:

> Yeah, my default editor configuration for AsciiDoc is two spaces.  I
> noticed that Junio's already picked it up for next, but I'll send a v2
> with this fixed in case he wants to merge the fixed version to master
> instead.
>
> If it's more convenient, I can send a follow-up patch fixing the whitespace.

Either is fine.  At this point in the cycle where we are very close
to -rc0, I do not mind too much about having a few reverts of 'next'
to give a clean slate to an obvious and trivial fix.

Thanks.

Re: [RFC PATCH 10/10] pack-objects: improve partial packfile reuse

[Junio C Hamano]

Jeff King  writes:

>> Hmm, I am kind of surprised that the decoding side allowed such a
>> padding.
>
> IIRC, the "padding" is just a sequence of 0-length-plus-continuation-bit
> varint bytes. And for some reason it worked for the size but not for the
> delta offset value.

I think the reason is because they use different varint definition.

The encoding used in builtin/pack-objects.c::write_no_reuse_object()
is for offsets, and it came much later and with an improvement over
the encoding used for delta size in diff-delta.c::create_delta().
The more recent encoding does not allow padding (when I compare the
decoders for these two encodings, I notice there is +1 for each
7-bit iteration; this essentially declares that a byte with "not the
final byte" bit set with all other bits clear does not mean 0 but it
means 1, which breaks the idea of padding to encode filler zero
bits).

Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

[Birger Skogeng Pedersen]

Hi Johannes,

On Thu, Oct 17, 2019 at 7:33 AM Johannes Sixt  wrote:
> FWIW, I would prefer to experiment with the feature for a few weeks
> before it (or a configuration that enables it by default) is baked in.

Yes please do. Obviously I'm glad someone other than me will be
actually testing it.
(As I mentioned earlier) I'm all for it when it comes to using a
config setting, rather than having this as default behaviour. I
propose "gui.autoFocusStaged" as a variable name for the setting.
I'll be doing a re-roll once I get some more spare time.

Birger

Re: Git Test Coverage Report (October 11)

[Jeff King]

On Fri, Oct 11, 2019 at 09:33:11AM -0400, Derrick Stolee wrote:

> Here is today's test coverage report. The usual report format is
> available online [1], [2]. The report listed below is a new format
> that groups lines by the commit that introduced them [3]. Thanks
> Peff for the feedback on that idea.

Thanks. FWIW, I did find this one easier to scan through looking for my
own bits.

-Peff

Re: [PATCH 6/6] index-pack: make quantum of work smaller

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:22PM -0700, Jonathan Tan wrote:

> Signed-off-by: Jonathan Tan 
> ---
>  builtin/index-pack.c | 267 ---
>  1 file changed, 127 insertions(+), 140 deletions(-)

I think this is a good direction to go in. I confess I didn't carefully
go over the implementation details, since you've marked this as RFC and
it sounds like you're mainly asking about direction. It looks pretty
reasonable from a high level, though.

-Peff

Re: [PATCH 5/6] index-pack: make resolve_delta() assume base data

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:21PM -0700, Jonathan Tan wrote:

> A subsequent commit will make the quantum of work smaller, necessitating
> more locking. This commit allows resolve_delta() to be called outside
> the lock.

OK, that makes sense, I think.

-Peff

Re: [PATCH 4/6] index-pack: calculate {ref,ofs}_{first,last} early

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:20PM -0700, Jonathan Tan wrote:

> Whenever we make a struct base_data, immediately calculate its delta
> children. This eliminates confusion as to when the
> {ref,ofs}_{first,last} fields are initialized.

That _seems_ like a good idea, but I'm a little worried just because I
don't entirely understand why it was being done lazily before. If you've
puzzled all that out, it would be nice to make the argument in the
commit message.

-Peff

Re: [PATCH 3/6] index-pack: remove redundant child field

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:19PM -0700, Jonathan Tan wrote:

> -static void prune_base_data(struct base_data *retain)
> +static void prune_base_data(struct base_data *youngest_child)
>  {
>   struct base_data *b;
>   struct thread_local *data = get_thread_data();
> - for (b = data->base_cache;
> -  data->base_cache_used > delta_base_cache_limit && b;
> -  b = b->child) {
> - if (b->data && b != retain)
> - free_base_data(b);
> + struct base_data **ancestry = NULL;
> + int nr = 0, alloc = 0;

Minor, but please use size_t for allocation variables.

> + int i;

Technically this probably ought to be a size_t as well, but I'm much
more concerned about the allocation ones, where we might accidentally
overflow and underallocate a buffer. Overflowing "i" would probably just
lead to an error or bad result.

I _think_ what the patch is actually doing makes sense (taking as an
assumption that it's heading in a useful direction for the end of the
series).

-Peff

Re: [PATCH 3/6] index-pack: remove redundant child field

[Jeff King]

On Thu, Oct 10, 2019 at 12:02:29PM -0700, Jonathan Tan wrote:

> > On 10/9/2019 7:44 PM, Jonathan Tan wrote:
> > > Instead, recompute ancestry if we ever need to reclaim memory.
> > 
> > I find this message lacking in important details:
> > 
> > 1. Where do we recompute ancestry?
> > 2. What are the performance implications of this change?
> > 3. Why is it important that you construct a stack of deltas in 
> > prune_base_data()?
> 
> Thanks for taking a look at this. My original plan (as I perhaps badly
> explained in the cover letter [1]) was to show the individual small
> steps that I took to reach the end goal, each step still passing all
> tests, in the hope that small steps are easier to understand than one
> big one. Hence why I didn't explain much in this commit message (and
> others), because I thought that I might have to squash them later. But
> perhaps that is too confusing and I should have just squashed them in
> the first place (and explain all the changes in the commit message -
> it's +177 -198, which is not too big anyway).

FWIW, I like the breakdown. These are tricky cleanups, and seeing them
individually makes it easy to verify that they don't themselves break
anything. I think they just need more explanation.

-Peff

Re: [PATCH 2/6] index-pack: remove redundant parameter

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:18PM -0700, Jonathan Tan wrote:

> Signed-off-by: Jonathan Tan 
> ---
>  builtin/index-pack.c | 26 --
>  1 file changed, 12 insertions(+), 14 deletions(-)

Yeah, this seems like a good cleanup. Probably worth explaining in the
commit message that we can infer it from the functions themselves.

-Peff

Re: [PATCH 1/6] index-pack: unify threaded and unthreaded code

[Jeff King]

On Wed, Oct 09, 2019 at 04:44:17PM -0700, Jonathan Tan wrote:

> Signed-off-by: Jonathan Tan 
> ---
>  builtin/index-pack.c | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a23454da6e..1a2600d4b3 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1210,15 +1210,7 @@ static void resolve_deltas(void)
>   cleanup_thread();
>   return;
>   }
> -
> - for (i = 0; i < nr_objects; i++) {
> - struct object_entry *obj = &objects[i];
> -
> - if (is_delta_type(obj->type))
> - continue;
> - resolve_base(obj);
> - display_progress(progress, nr_resolved_deltas);
> - }
> + threaded_second_pass(¬hread_data);
>  }

I wondered at first if this would be a problem with NO_PTHREADS. But I
think since the cleanups around 2094c5e582 (index-pack: remove #ifdef
NO_PTHREADS, 2018-11-03), we always define threaded_second_pass(), even
if all the locks are noops and we only ever have one thread.

-Peff

Re: [PATCH v2] send-pack: never fetch when checking exclusions

[Jeff King]

On Fri, Oct 11, 2019 at 03:08:22PM -0700, Jonathan Tan wrote:

> > As a general rule (and why I'm raising this issue in reply to Jonathan's
> > patch), I think most or all sites that want OBJECT_INFO_QUICK will want
> > SKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally
> > the same:
> > 
> >   - it's OK to racily have a false negative (we'll still be correct, but
> > possibly a little less optimal)
> > 
> >   - it's expected and normal to be missing the object, so spending time
> > double-checking the pack store wastes measurable time in real-world
> > cases
> 
> I took a look on "next" and it's true for these reasons in most cases
> but not all.

Thanks for digging into this.

> QUICK implies SKIP_FETCH_OBJECT:
> 
>   fetch-pack.c: Run with fetch_if_missing=0 (from builtin/fetch.c,
>   builtin/fetch-pack.c, or through a lazy fetch) so OK.
>   
>   builtin/index-pack.c: Run with fetch_if_missing=0, so OK.
>   
>   builtin/fetch.c: Run with fetch_if_missing=0, so OK.
>   
>   object-store.h, sha1-file.c: Definition and implementation of this
>   flag.

Right, I think going in this direction is pretty simple. Having been
marked with QUICK, they hit both of my points from above. And if we want
to avoid re-scanning the pack directory because of cost, we _definitely_
want to avoid making an expensive network call.

> Everything is OK here. Now, SKIP_FETCH_OBJECT implies QUICK:
> 
>   cache-tree.c: I added this recently in f981ec18cf ("cache-tree: do not
>   lazy-fetch tentative tree", 2019-09-09). No problem with a false
>   negative, since we know how to reconstruct the tree. OK.
> [...]
>   send-pack.c: This patch (which is already in "next"). If we have a
>   false negative, we might accidentally send more than we need. But that
>   is not too bad.

Yeah, I think both of these could be QUICK.

>   promisor-remote.c: This is the slightly tricky one. We use this
>   information to determine if we got our lazily-fetched object from the
>   most recent lazy fetch, or if we should continue attempting to fetch the
>   given object from other promisor remotes; so this information is
>   important. However, adding QUICK doesn't lose us anything because the
>   lack of QUICK only helps us when there is another process packing
>   loose objects: if we got our object, our object will be in a pack
>   (because of the way the fetch is implemented - in particular, we need
>   a pack because we need the ".promisor" file).
> 
> So everything is OK except for promisor-remote.c, but even that is OK
> for another reason.

Yeah, though I wouldn't be sad to see that use a separate flag, since it
really is about promisor logic.

That implies to me maybe we should be using QUICK more aggressively, and
QUICK should auto-imply SKIP_FETCH_OBJECT.

> Having said that, perhaps we should consider promisor-remote.c as
> low-level code and expect it to know that objects are fetched into a
> packfile (as opposed to loose objects), so it can safely use QUICK
> (which is documented as checking packed after packed and loose). If no
> one disagrees, I can make such a patch after jt/push-avoid-lazy-fetch is
> merged to master (as is the plan, according to What's Cooking [1]).

I think it's OK to continue leaving out QUICK there if it's not causing
problems. It really is a bit different than the other cases.

-Peff

Re: git smart http + apache mod_auth_openidc

[Jeff King]

On Thu, Oct 17, 2019 at 03:00:58AM +, Ralph Ewig wrote:

> Thanks for the reply. I was hoping the Git GUI 
> might be able to handle the OpenID authentication 
> flow, but it makes sense that it would be 
> inconsistent with other git clients.

I don't think we'd ever do the full flow, but it might be that it's
possible to use a token generated by the flow in a separate browser
session. This is easy to do if you can provide the token to the server
in place of a password. E.g., if you use 2FA on GitHub, you can generate
a limited token that allows you to connect for Git operations.

You can also use time-limited tokens and feed them from a custom
credential helper, which does the full authentication flow from a web
browser. I think Google does (or did) have such a thing internally, but
I don't think any part of it was made public (it was discussed on the
list a couple times in the early days of credential helpers).

That would still have to end up with a token "password" to send over
basic auth, I think. There was some discussion a while back of letting
credential helpers pass back content for HTTP "Authorization" headers,
but I don't think anything was ever merged.

> >>       C:\Users\void>git clone --progress -v
> >> "https://git.xxx.xxx/WebApps.git";
> >>       Cloning into 'WebApps'...
> >>       fatal: unable to update url base from
> >> redirection:
> >>     asked for:
> >> https://git.xxx.xxx/WebApps.git/info/refs?service=git-upload-pack
> >>      redirect:
> >> https://login.microsoftonline.com/xxx/oauth2/authorize?response_type=code&scope=openid&client_id=xxx&state=xxx&redirect_uri=https%3A%2F%2Fgit.xxx.xxx%2Fredirect&nonce=xxx

You're running into one other complexity here, which is that we don't
allow cross-server redirects during the initial conversation. So even if
you did have an auth flow where we could somehow provide all the
information via Git, we'd still be unhappy about bouncing between
multiple servers.

Mostly that's there to avoid leaking credentials. We only have one
notion of "the username and password we're using" for a given fetch, so
we want to avoid leaking it if we're redirected to another server. But
obviously Git _could_ be smarter there, if this was the only blocker
remaining (but from my understanding of OpenID, it's not).

-Peff

Re: [PATCH v2 3/3] docs: mention when increasing http.postBuffer is valuable

[Jeff King]

On Thu, Oct 17, 2019 at 12:53:30AM +, brian m. carlson wrote:

> Users in a wide variety of situations find themselves with HTTP push
> problems.  Oftentimes these issues are due to antivirus software,
> filtering proxies, or other man-in-the-middle situations; other times,
> they are due to simple unreliability of the network.
> 
> However, a common solution to HTTP push problems found online is to
> increase http.postBuffer.  This works for none of the aforementioned
> situations and is only useful in a small, highly restricted number of
> cases: essentially, when the connection does not properly support
> HTTP/1.1.
> 
> Document when raising this value is appropriate and what it actually
> does, and discourage people from using it as a general solution for push
> problems, since it is not effective there.

Yeah, I've run into some voodoo advice about this config option before.
I think your advice neatly explains the situation.

-Peff

Re: [PATCH v2 2/3] doc: dissuade users from trying to ignore tracked files

[Jeff King]

On Thu, Oct 17, 2019 at 12:53:29AM +, brian m. carlson wrote:

> There is no sensible behavior in this case, because sometimes the data
> is precious, such as certain configuration files, and sometimes it is
> irrelevant data that the user would be happy to discard.
> 
> Since this is not a supported configuration and users are prone to
> misuse the existing features for unintended purposes, causing general
> sadness and confusion, let's document the existing behavior and the
> pitfalls in the documentation for git update-index so that users know
> they should explore alternate solutions.

I think this is an improvement. It would be nice if we had some specific
alternatives to point them to. But as you note, Git doesn't really have
a good solution for this built-in. And I don't know offhand of a
complete one that we could recommend. So this is probably the best we
can do for now.

-Peff

Re: [PATCH v2 1/3] doc: provide guidance on user.name format

[Jeff King]

On Thu, Oct 17, 2019 at 12:53:28AM +, brian m. carlson wrote:

> It's a frequent misconception that the user.name variable controls
> authentication in some way, and as a result, beginning users frequently
> attempt to change it when they're having authentication troubles.
> Document that the convention is that this variable represents some form
> of a human's personal name, although that is not required.  In addition,
> address concerns about whether Unicode is supported.
> 
> Use the term "personal name" as this is likely to draw the intended
> contrast, be applicable across cultures which may have different naming
> conventions, and be easily understandable to people who do not speak
> English as their first language.  Indicate that "some form" is
> conventionally used, as people may use a nickname or preferred name
> instead of a full legal name.
> 
> Point users who may be confused about authentication to an appropriate
> configuration option instead.

I think this is a good distinction to draw, but...

>  Documentation/git-commit-tree.txt | 6 ++
>  1 file changed, 6 insertions(+)

...I was surprised to see it here, where I think most users wouldn't
find it. Would it make sense in git-commit(1), or in the description of
the user.name config?

-Peff

Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

[Johannes Sixt]

Am 17.10.19 um 07:08 schrieb Birger Skogeng Pedersen:
> Hi Pratyush,
> 
> On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav  wrote:
>> I mentioned this earlier, and I'll mention this again: I'm not sure
>> whether this feature would be a good thing for the larger population. So
>> this _might_ not end up being accepted depending on how people react to
>> the proposal. I thought I'd let you know to avoid any nasty surprises
>> later.
> 
> Could you please elaborate on why you think the feature might be
> undesired? Why would users not want a staged file to be selected
> automatically?

FWIW, I would prefer to experiment with the feature for a few weeks
before it (or a configuration that enables it by default) is baked in.

-- Hannes

Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

[Birger Skogeng Pedersen]

Hi Pratyush,

On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav  wrote:
> I mentioned this earlier, and I'll mention this again: I'm not sure
> whether this feature would be a good thing for the larger population. So
> this _might_ not end up being accepted depending on how people react to
> the proposal. I thought I'd let you know to avoid any nasty surprises
> later.

Could you please elaborate on why you think the feature might be
undesired? Why would users not want a staged file to be selected
automatically?

FWIW I've also got 2 comments on this in GH[1].

[1] https://github.com/git-for-windows/git/issues/2341

Best regards,
Birger

Re: email as a bona fide git transport

[Junio C Hamano]

Vegard Nossum  writes:

> Step 1:
>
> * git send-email needs to include parent SHA1s and generally all the
>   information needed to perfectly recreate the commit when applied so
>   that all the SHA1s remain the same
>
> * git am (or an alternative command) needs to recreate the commit
>   perfectly when applied, including applying it to the correct parent

You can record and convey the commit object name a series is meant
to be applied on top already, and it in general is a good way to
give a wider context in order to explain and justify the series.

On the other hand, "all the information needed to recreate..." is
not very useful.  If you want the commit object to be exactly what
you want to see at the tip of the end result, you are better off
asking your upstream to pull.  Using e-mail for that makes you and
project participants give up a lot of benefits the workflow based on
e-mail gives you, the biggest of which is the ease of giving
suggestions for improvements.  Once you insist "perfectly recreate
the commit", you are not willing to take any input from the
sidelines---worse yet, you are even dictating when the upstream
runs "git am" to turn them into commits, and do so without reading
the patches (there is no point reviewing as the person who runs "git
am" is not even allowed to fix typo or make obvious fixes to the
code, which will fail to perfectly recreate the commit).

In short, one should resist temptation to bring up "perfect
reproduction" when one talks about e-mail workflow.

> * Instead of describing a patchset in a separate introduction email, we
>   can create a merge commit between the parent of the first commit in
>   the series and the last and put the patchset description in the merge
>   commit [5]. This means the patchset description also gets to be part
>   of git history.

This has been done with tools around git-core, and merits a more
official support.  When merging a topic, it is a good idea to
explain in the merge commit that brings in the topic to the mainline
what the topic is about, and at least in the past few years Linus
and other maintainers both within and outside the kernel have been
doing so.  The cover-letter material in [PATCH 00/NN] obviously can
help those integrators when they write the merge message.

>   (This would require support for git send-email/am to be able to send
>   and apply merge commits -- at least those which have the same tree as
>   one of the parents. This is _not_ yet supported in my proposed git
>   patches.)

This does not require much from format-patch and am.  All you need
to do is to ensure that they can handle an empty commit.  What you
need more is a support in merge.  The outline for the workflow would
go like this:

 * The contributor prepares an N patch series 1/N..N/N on a single
   topic branch.

 * The summary of the series, the message that is meant to help the
   integrator, is recorded as (N+1)th commit at the tip of the topic
   branch, as an empty commit (i.e. a commit that records the same
   tree as its parent).

 * "git format-patch" is taught, when told to prepare the patch
   e-mails from such a topic branch, to notice the unusual "an empty
   commit at the tip" layout, and turn that into 0/N of the message.

 * "git am" is taught a new option to cap a topic branch made from
   patches 1/N..N/N from the incoming mbox with an extra empty
   commit, whose message is taken from the 0/N cover-letter
   material, to recreate what the contributor had in the second step
   above.

 * "git merge" is taught, when told to merge a topic branch, to
   notice the unusual "an empty commit at the tip" layout, and

   (1) merge topic~1 instead to excise the empty commit itself,

   (2) take the log message from the empty commit at the tip and use
   it to help prepare the log message of the merge.

Re: git smart http + apache mod_auth_openidc

[Ralph Ewig]

Thanks for the reply. I was hoping the Git GUI 
might be able to handle the OpenID authentication 
flow, but it makes sense that it would be 
inconsistent with other git clients.

Azure AD does support both LDAP and Kerberos, but 
unfortunately only as an extra cost add-on called 
"Domain Services". I might try to hack up a script 
to just sync the Azure AD password hashes to the 
htpassword file. Otherwise I guess I'll have to 
bite the bullet and pay the extra bill.

Nonetheless, thank you for the suggestion!
Ralph


On 10/16/2019 4:33 PM, brian m. carlson wrote:
> On 2019-10-15 at 15:59:03, Ralph Ewig wrote:
>> Hi Everyone, hoping you might have a solution for
>> this problem:
>>
>> CONTEXT
>>
>>    * I'm serving git repos using "smart https" via
>> apache and basic authentication; everything works
>> fine.
>>    * We're moving to SSO via Azure AD and apache
>> mod_auth_openidc; this works fine for gitweb (on
>> the same server as git).
>>
>> PROBLEM
>>
>> When trying to clone a repo with the OIDC setup,
>> git breaks on the redirect for user authentication
>> with the following error message (replaced server
>> ids etc with xxx):
>>
>>       C:\Users\void>git clone --progress -v
>> "https://git.xxx.xxx/WebApps.git";
>>       Cloning into 'WebApps'...
>>       fatal: unable to update url base from
>> redirection:
>>     asked for:
>> https://git.xxx.xxx/WebApps.git/info/refs?service=git-upload-pack
>>      redirect:
>> https://login.microsoftonline.com/xxx/oauth2/authorize?response_type=code&scope=openid&client_id=xxx&state=xxx&redirect_uri=https%3A%2F%2Fgit.xxx.xxx%2Fredirect&nonce=xxx
>>
>>
>> Can the git client just not handle a web based
>> redirect for login, or is this a configuration
>> issue? Any ideas would be greatly appreciated. Thanks!
> The Git client doesn't handle any sort of web-based login.  In general,
> in order to do web-based login, you have to provide a fully functional
> graphical web browser, and Git operates in many environments that don't
> have one (such as servers, containers, and headless systems).
>
> You should treat your Git server like you would treat any API you may
> access, since essentially it is one.  That means that you would need to
> provide a way to use some sort of external credential.
>
> I know next to nothing about Azure AD, but it claims to support
> Kerberos, so you may be able to use that in conjunction with libcurl's
> GSS-Negotiate support and Apache's mod_auth_kerb (which is shipped in
> Debian).  I use Kerberos-based authentication for my personal server
> (which is Linux, not AD) and it does work, so it is possible to set up.

Re: [PATCH v2] Doc: Bundle file usage

[Junio C Hamano]

Jeff King  writes:

> Maybe:
>
>   'git fetch', 'git pull', and 'git clone'
>
> ? Given the repetition below, though:

... including this one, I think you covered everything I wanted to
say on the patch already.  Thanks.

Re: [PATCH] remote-curl: pass on atomic capability to remote side

[brian m. carlson]

On 2019-10-15 at 16:40:29, Jeff King wrote:
> On Tue, Oct 15, 2019 at 01:07:59AM +, brian m. carlson wrote:
> > diff --git a/Documentation/gitremote-helpers.txt 
> > b/Documentation/gitremote-helpers.txt
> > index a5c3c04371..670d72c174 100644
> > --- a/Documentation/gitremote-helpers.txt
> > +++ b/Documentation/gitremote-helpers.txt
> > @@ -509,6 +509,11 @@ set by Git if the remote helper has the 'option' 
> > capability.
> > Indicate that only the objects wanted need to be fetched, not
> > their dependents.
> >  
> > +'option atomic' {'true'|'false'}::
> > +  When pushing, request the remote server to update refs in a single atomic
> > +  transaction.  If successful, all refs will be updated, or none will.  If 
> > the
> > +  remote side does not support this capability, the push will fail.
> > +
> 
> This is implemented with a single space, but the rest of the option
> bodies are indented with a tab. Asciidoc seems to format it identically
> either way, though.

Yeah, my default editor configuration for AsciiDoc is two spaces.  I
noticed that Junio's already picked it up for next, but I'll send a v2
with this fixed in case he wants to merge the fixed version to master
instead.

If it's more convenient, I can send a follow-up patch fixing the whitespace.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: git smart http + apache mod_auth_openidc

[brian m. carlson]

On 2019-10-15 at 15:59:03, Ralph Ewig wrote:
> Hi Everyone, hoping you might have a solution for 
> this problem:
> 
> CONTEXT
> 
>   * I'm serving git repos using "smart https" via 
> apache and basic authentication; everything works 
> fine.
>   * We're moving to SSO via Azure AD and apache 
> mod_auth_openidc; this works fine for gitweb (on 
> the same server as git).
> 
> PROBLEM
> 
> When trying to clone a repo with the OIDC setup, 
> git breaks on the redirect for user authentication 
> with the following error message (replaced server 
> ids etc with xxx):
> 
>      C:\Users\void>git clone --progress -v 
> "https://git.xxx.xxx/WebApps.git";
>      Cloning into 'WebApps'...
>      fatal: unable to update url base from 
> redirection:
>    asked for: 
> https://git.xxx.xxx/WebApps.git/info/refs?service=git-upload-pack
>     redirect: 
> https://login.microsoftonline.com/xxx/oauth2/authorize?response_type=code&scope=openid&client_id=xxx&state=xxx&redirect_uri=https%3A%2F%2Fgit.xxx.xxx%2Fredirect&nonce=xxx
> 
> 
> Can the git client just not handle a web based 
> redirect for login, or is this a configuration 
> issue? Any ideas would be greatly appreciated. Thanks!

The Git client doesn't handle any sort of web-based login.  In general,
in order to do web-based login, you have to provide a fully functional
graphical web browser, and Git operates in many environments that don't
have one (such as servers, containers, and headless systems).

You should treat your Git server like you would treat any API you may
access, since essentially it is one.  That means that you would need to
provide a way to use some sort of external credential.

I know next to nothing about Azure AD, but it claims to support
Kerberos, so you may be able to use that in conjunction with libcurl's
GSS-Negotiate support and Apache's mod_auth_kerb (which is shipped in
Debian).  I use Kerberos-based authentication for my personal server
(which is Linux, not AD) and it does work, so it is possible to set up.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2] userdiff: Fix some corner cases in dts regex

[Johannes Sixt]

[Removed bouncing addresses of Matthieu Moy and William Duclot from Cc]

Am 16.10.19 um 22:32 schrieb Stephen Boyd:
> diff --git a/t/t4018/dts-nodes-multiline-prop 
> b/t/t4018/dts-nodes-multiline-prop
> new file mode 100644
> index ..db4b4bdda686
> --- /dev/null
> +++ b/t/t4018/dts-nodes-multiline-prop
> @@ -0,0 +1,14 @@
> +/ {
> + label_1: node1@ff00 {
> + RIGHT@deadf00,4000 {
> + multilineprop = <3>,
> +
> +
> + <4>;

I was actually thinking about something like

multilineprop = <3>,
<0xabcd>,
"text",
name,
<4>;

or something like that -- whatever occurs in the real world.

> +
> +
> +
> + ChangeMe = <0xffeedd00>;
> + };
> + };
> +};

Apart from that, the patch looks good.

-- Hannes

Re: [PATCH v2] Doc: Bundle file usage

[Jeff King]

On Wed, Oct 16, 2019 at 10:57:37AM +0100, Philip Oakley wrote:

> From: Philip Oakley 
> 
> Git URLs can accept bundle files for fetch, pull and clone, include
> in that section. Include git clone in the bundle usage description.
> Correct the quoting of .
> Detail the  options for cloning a complete repo.

Thanks for picking this up again. :)

A few minor comments:

> diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
> index 7d6c9dcd17..0498e4895d 100644
> --- a/Documentation/git-bundle.txt
> +++ b/Documentation/git-bundle.txt
> @@ -21,9 +21,9 @@ Some workflows require that one or more branches of 
> development on one
>  machine be replicated on another machine, but the two machines cannot
>  be directly connected, and therefore the interactive Git protocols (git,
>  ssh, http) cannot be used.  This command provides support for
> -'git fetch' and 'git pull' to operate by packaging objects and references
> +'git fetch' and 'git pull' and 'git clone', to operate by packaging objects 
> and references

Maybe:

  'git fetch', 'git pull', and 'git clone'

? Given the repetition below, though:

>  in an archive at the originating machine, then importing those into
> -another repository using 'git fetch' and 'git pull'
> +another repository using 'git fetch' and 'git pull' or 'git clone',

I wonder if we could rephrase this in a less awkward way. Perhaps:

  The 'git bundle' command packages objects and references in an archive
  at the originating machine, which can then be imported into another
  repository using 'git fetch', 'git pull', or 'git clone'.

> @@ -35,7 +35,7 @@ OPTIONS
>  
>  create ::
>   Used to create a bundle named 'file'.  This requires the
> - 'git-rev-list-args' arguments to define the bundle contents.
> + '' arguments to define the bundle contents.

This hunk makes sense. I'd probably use backticks here instead of
single-quotes, but I think we're pretty inconsistent across the
documentation about this. It probably makes sense to match the
existing text.

> @@ -92,6 +92,10 @@ It is okay to err on the side of caution, causing the 
> bundle file
>  to contain objects already in the destination, as these are ignored
>  when unpacking at the destination.
>  
> +To create a bundle for 'git clone', use `--branches --tags` for
> +the . The (inappropriate) use of `--all` would include
> +refs from refs/remotes/* hierarchy in the resulting bundle.

Should  be in quotes or backticks?

Any bundle created without a negative revision would be appropriate for
a clone. Maybe we could spell that out in more detail, like:

  Any bundle created without negative refspecs (e.g., `new` but not
  `old..new`) can be used on the receiving side with `git clone`. If you
  want to provide the same set of refs that a clone directly from the
  source repository would get, use `--branches --tags`. If you want to
  match `git clone --mirror`, which would clone other refs such as
  `refs/remotes/*`, use `--all`.

> diff --git a/Documentation/urls.txt b/Documentation/urls.txt
> index bc354fe2dc..1c229d7581 100644
> --- a/Documentation/urls.txt
> +++ b/Documentation/urls.txt
> @@ -53,6 +53,9 @@ These two syntaxes are mostly equivalent, except the former 
> implies
>  --local option.
>  endif::git-clone[]
>  
> +'git clone', 'git fetch' and 'git pull', but not 'git push', will also
> +accept a suitable bundle file. See linkgit:git-bundle[1].

This makes sense to mention here. It's a little funny because the user
would see this included in "man git-clone" or whatever, but I don't
think it hurts to just be exhaustive rather than trying to tailor it to
each individual manpage.

-Peff

Re: email as a bona fide git transport

[Jonathan Nieder]

Hi,

A few small points.

Vegard Nossum wrote:

> * git am (or an alternative command) needs to recreate the commit
>   perfectly when applied, including applying it to the correct parent

Interesting.  "git format-patch" has a --base option to do some of
what you're looking for, for the sake of snowpatch
.  Though it's not exactly the
same thing you mean.

We also discussed sending merge commits by mail recently in the
virtual git committer summit[1].

Of course, the devil is in the details.  It's straightforward to use
"git bundle" to use mail as a Git transport today, but presumably you
also want the ability to perform reviews along the way and that's not
so easy with a binary format.  Do you have more details on what you'd
want the format to look like, particularly for merge commits?

[...]
> there
> is no need for "changeset IDs" or whatever, since you can just use the
> git SHA1 which is unique, unambiguous, and stable.

In [2] the hope was for some identifier that is preserved by "git
rebase" and "git commit --amend" (so that you can track the evolution
of a change as the author improves it in response to reviews).  Is
that the conversation you're alluding to?

[...]
> Disadvantages:
>
> - requires patching git

That's not a disadvantage.  It means get to work with the Git project,
which is a welcoming bunch of people, working on userspace (seeing how
the other half lives), and improving the lives of everyone using Git.

[...]
> Date: Sat, 5 Oct 2019 16:15:59 +0200
> Subject: [PATCH 1/3] format-patch: add --complete
>
> Include the raw commit data between the changelog and the diffstat.

Oh!  I had missed this on first reading because it was in an
attachment.

I have mixed feelings.  Can you say a bit more about the advantages
and disadvantages relative to sending a git bundle?  What happens if a
mail client or a box along the way mangles whitespace in the commit
message?

Happy hacking,
Jonathan

[1] 
https://public-inbox.org/git/nycvar.qro.7.76.6.1909261253400.15...@tvgsbejvaqbjf.bet/
[2] 
https://lore.kernel.org/ksummit-discuss/CAD=FV=upjppuyftpjf-ogzj_6ljle4ptxmhcocedmh1lxss...@mail.gmail.com/

Re: [PATCH] userdiff: Fix some corner cases in dts regex

[Stephen Boyd]

Quoting Johannes Sixt (2019-10-12 05:54:00)
> Am 08.10.19 um 16:43 schrieb Stephen Boyd:
> > Quoting Johannes Sixt (2019-10-05 07:09:11)
> >> Am 04.10.19 um 23:30 schrieb Stephen Boyd:
> >>> --- /dev/null
> >>> +++ b/t/t4018/dts-nodes-multiline-prop
> >>> @@ -0,0 +1,12 @@
> >>> +/ {
> >>> + label_1: node1@ff00 {
> >>> + RIGHT@deadf00,4000 {
> >>> + multilineprop = <3>,
> >>> + <4>;
> >>
> >> You could insert more lines to demonstrate that "," on a line by
> >> itself is not picked up.
> > 
> > Maybe I should add another test?
> 
> This is is the _multi_line test case, right? ;) Just add one or two
> lines between the <3> and the <4> that look like common real-world cases
> to show that those lines won't be picked up. I don't think that another
> test file is required.

Ok got it!

> 
> >>> +/ { RIGHT /* Technically just supposed to be a slash and brace */
> >>
> >> Devil's advocate here: insert ';' or '=' in the comment, and the line
> >> would not be picked up. Does that hurt in practice?
> > 
> > I don't think it hurts in practice so I'd like to ignore it.
> 
> Sure, no problem.
> 
> >>>  PATTERNS("dts",
> >>>"!;\n"
> >>> +  "!.*=.*\n"
> >>
> >> This behaves the same way as just
> >>
> >> "!=\n"
> >>
> >> no?
> >>
> > 
> > Not exactly. Properties don't always get assigned.
> 
> I was just refering to the added line, not the combination of the two lines.

Ah ok. I'll reduce the line as you suggest then. Thanks.

> 
> But while you are speaking of it:
> 
> > There are boolean
> > properties that can be tested for by the presence of some string with an
> > ending semi-colon, like 'this-is-true;'. If we just check for not equal
> > to a line with a semicolon and newline then we'll see boolean
> > properties. Should I add that as another test?
> 
> I agree that a test case with a Boolean property would be great.
> 

Alright I'll work on that and resend.

Re: [PATCH v4 00/17] New sparse-checkout builtin and "cone" mode

[Elijah Newren]

On Tue, Oct 15, 2019 at 6:56 AM Derrick Stolee via GitGitGadget
 wrote:
> Updates in V4:
>
>  * Updated hashmap API usage to respond to ew/hashmap
>
>
>  * Responded to detailed review by Elijah. Thanks!
>
>
>  * Marked the feature as experimental in git-sparse-checkout.txt the same
>way that git-switch.txt does.

I read through the range-diff, and it all looks good to me other than
one issue I flagged on patch 1.

Nice work!

Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag

[William Baker]

On 10/15/19 7:09 PM, Junio C Hamano wrote:
>> At this time there are no other MIDX_* flags and so there's always the option
>> to revisit the best way to handle multiple MIDX_* flags if/when additional
>> flags are added.
> 
> I do not care too deeply either way, but if you wrote it in one way,
> how about completing the series without changing it in the middle,
> and leave the clean-ups to a follow-up series (if needed)?


That plan sounds good to me.  The most recent series (v3) should be ready
to go, I don't believe there is any outstanding feedback to address.

Thanks,
William

Re: [PATCH 1/1] builtin/blame.c: constants into bit shift format

[Pratyush Yadav]

On 16/10/19 12:37PM, Jonathan Tan wrote:
> > There was some discussion recently about converting these related 
> > #defines to enums [0]. We might consider doing that here.
> > 
> > If you read through that entire thread, you'd see that there were some 
> > disagreements about whether using enums for sets of bits is a good idea 
> > ([1] and [2]), but it is at least something worth considering while we 
> > are on this topic.
> > 
> > FWIW, I think it is a good idea to use an enum here.
> 
> [snip]
> 
> > [0] 
> > https://public-inbox.org/git/20191010115230.10623-1-wambui.karu...@gmail.com/
> > [1] 
> > https://public-inbox.org/git/20191014182754.82302-1-jonathanta...@google.com/
> > [2] https://public-inbox.org/git/xmqqk19ag60g@gitster-ct.c.googlers.com/
> 
> Thanks for the handy references. You know my opinion on bitflags as
> enums from reading them, but I think that we have already had that
> discussion and came to a conclusion. So don't use an enum here.

Ah! I missed your last email in that thread that finally settled on 
avoiding bitsets, and thought the discussion was still ongoing. My bad 
:)
 
> The patch itself looks good, and I also prefer the bit shift format over
> octal.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/1] builtin/blame.c: constants into bit shift format

[Jonathan Tan]

> There was some discussion recently about converting these related 
> #defines to enums [0]. We might consider doing that here.
> 
> If you read through that entire thread, you'd see that there were some 
> disagreements about whether using enums for sets of bits is a good idea 
> ([1] and [2]), but it is at least something worth considering while we 
> are on this topic.
> 
> FWIW, I think it is a good idea to use an enum here.

[snip]

> [0] 
> https://public-inbox.org/git/20191010115230.10623-1-wambui.karu...@gmail.com/
> [1] 
> https://public-inbox.org/git/20191014182754.82302-1-jonathanta...@google.com/
> [2] https://public-inbox.org/git/xmqqk19ag60g@gitster-ct.c.googlers.com/

Thanks for the handy references. You know my opinion on bitflags as
enums from reading them, but I think that we have already had that
discussion and came to a conclusion. So don't use an enum here.

The patch itself looks good, and I also prefer the bit shift format over
octal.

Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

[Pratyush Yadav]

On 15/10/19 12:51PM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> Thanks for reviewing. How does this work, do I send a re-roll of the 
> patch(es)?

Yes, please do.

I mentioned this earlier, and I'll mention this again: I'm not sure 
whether this feature would be a good thing for the larger population. So 
this _might_ not end up being accepted depending on how people react to 
the proposal. I thought I'd let you know to avoid any nasty surprises 
later.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/2] git-gui: select staged on ui_comm focus

[Pratyush Yadav]

On 07/10/19 07:11PM, Birger Skogeng Pedersen wrote:
> When the user focuses the Commit Message widget (to write a message), the
> diff view may be blank.
> 
> With this patch a staged file is automatically selected when the Commit
> Message widget is focused, if no other file is selected (i.e. diff view
> is blank).
> 
> Signed-off-by: Birger Skogeng Pedersen 
> ---
>  git-gui.sh | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index b7f4d1e..70b846a 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2700,6 +2700,15 @@ proc toggle_commit_type {} {
>   do_select_commit_type
>  }
>  
> +proc check_diff_selected {} {
> + global current_diff_path file_lists
> + # If no diff path selected, select a staged file
> + if {$current_diff_path eq {}
> + && [llength $file_lists($::ui_index)] > 0} {

Nitpick: Please declare ui_index to be global. That way we can just use 
$ui_index instead of the more tedious $::ui_index.

> + select_path_in_widget $::ui_index
> + }
> +}
> +
>  ##
>  ##
>  ## ui construction
> @@ -3437,6 +3446,8 @@ pack .vpane.lower.commarea.buffer.header -side top 
> -fill x
>  pack .vpane.lower.commarea.buffer.frame -side left -fill y
>  pack .vpane.lower.commarea.buffer -side left -fill y
>  
> +bind $ui_comm  {check_diff_selected}
> +

This would mean the diff shows _only_ when you switch focus to the 
commit message buffer. If the buffer is already in focus, and you stage 
all files via Ctrl-I, the staged diff would not show.

IIRC you were having some trouble with this. A quick suggestion without 
looking too much into the problem is to try putting the logic inside 
`do_add_all` instead of inside the bind event handler.

>  # -- Commit Message Buffer Context Menu
>  #
>  set ctxm .vpane.lower.commarea.buffer.ctxm
> -- 
> 2.23.0.windows.1
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/1] builtin/blame.c: constants into bit shift format

[Pratyush Yadav]

On 16/10/19 06:30PM, Hariom Verma via GitGitGadget wrote:
> From: Hariom Verma 
> 
> We are looking at bitfield constants, and elsewhere in the Git source
> code, such cases are handled via bit shift operators rather than octal
> numbers, which also makes it easier to spot holes in the range
> (if, say, 1<<5 was missing, it is easier to spot it between 1<<4
> and 1<<6 than it is to spot a missing 040 between a 020 and a 0100).
> 
> Signed-off-by: Hariom Verma 
> ---
>  builtin/blame.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e946ba6cd9..a57020acf9 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -319,18 +319,18 @@ static const char *format_time(timestamp_t time, const 
> char *tz_str,
>   return time_buf.buf;
>  }
>  
> -#define OUTPUT_ANNOTATE_COMPAT   001
> -#define OUTPUT_LONG_OBJECT_NAME  002
> -#define OUTPUT_RAW_TIMESTAMP 004
> -#define OUTPUT_PORCELAIN 010
> -#define OUTPUT_SHOW_NAME 020
> -#define OUTPUT_SHOW_NUMBER   040
> -#define OUTPUT_SHOW_SCORE0100
> -#define OUTPUT_NO_AUTHOR 0200
> -#define OUTPUT_SHOW_EMAIL0400
> -#define OUTPUT_LINE_PORCELAIN01000
> -#define OUTPUT_COLOR_LINE02000
> -#define OUTPUT_SHOW_AGE_WITH_COLOR   04000
> +#define OUTPUT_ANNOTATE_COMPAT  (1<<0)
> +#define OUTPUT_LONG_OBJECT_NAME (1<<1)
> +#define OUTPUT_RAW_TIMESTAMP(1<<2)
> +#define OUTPUT_PORCELAIN(1<<3)
> +#define OUTPUT_SHOW_NAME(1<<4)
> +#define OUTPUT_SHOW_NUMBER  (1<<5)
> +#define OUTPUT_SHOW_SCORE   (1<<6)
> +#define OUTPUT_NO_AUTHOR(1<<7)
> +#define OUTPUT_SHOW_EMAIL   (1<<8)
> +#define OUTPUT_LINE_PORCELAIN   (1<<9)
> +#define OUTPUT_COLOR_LINE   (1<<10)
> +#define OUTPUT_SHOW_AGE_WITH_COLOR  (1<<11)

Nitpick: In the code you remove, tabs were used for alignment. Here, you 
use spaces. Unless there is any specific reason to do it this way, might 
as well keep the older style.

There was some discussion recently about converting these related 
#defines to enums [0]. We might consider doing that here.

If you read through that entire thread, you'd see that there were some 
disagreements about whether using enums for sets of bits is a good idea 
([1] and [2]), but it is at least something worth considering while we 
are on this topic.

FWIW, I think it is a good idea to use an enum here.

>  
>  static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
>  {

[0] 
https://public-inbox.org/git/20191010115230.10623-1-wambui.karu...@gmail.com/
[1] 
https://public-inbox.org/git/20191014182754.82302-1-jonathanta...@google.com/
[2] https://public-inbox.org/git/xmqqk19ag60g@gitster-ct.c.googlers.com/

-- 
Regards,
Pratyush Yadav

Re: [RFC PATCH v1] t/README: the test repo does not have global or system configs

[Philip Oakley]

On 16/10/2019 17:47, SZEDER Gábor wrote:

On Wed, Oct 16, 2019 at 01:45:15PM +0100, Philip Oakley wrote:

Signed-off-by: Philip Oakley 
---

While tring to get to grips with some Git-for-Windows config settings
for testing >4GiB files, I couldn't find any note in the readme about
the test system config file sources.

The path of the system config file is determined at compile time, with
no way to override it at runtime.  Since we don't want external config
files influencing our tests, the only choice we have is to ignore the
system config file; that's why our test framework sets
GIT_CONFIG_NOSYSTEM=1.

Thanks.

Is this the right place for the information, is it complete enough,
and is the default config template special?

  t/README | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/t/README b/t/README
index 60d5b77bcc..3daa1fa182 100644
--- a/t/README
+++ b/t/README
@@ -485,6 +485,9 @@ This test harness library does the following things:
 the --root option documented above, and a '.stress-' suffix
 appended by the --stress option.
  
+ - The --global and --system config files are ignored, and

+   a basic --local config is created in the tst repository.

s/tst/test/

However, note that the global config file isn't really ignored, but
different.  The path of the global config file depends on the values
of the env variables $XDG_CONFIG_HOME and $HOME, and, again, to avoid
external influences, our test framework unsets the former, and
overrides the latter with HOME="$TRASH_DIRECTORY".  IOW the global
config file in our tests is '.../t/trash directory.t1234-foo/.gitconfig'.

Thanks

+
   - Defines standard test helper functions for your scripts to
 use.  These functions are designed to make all scripts behave
 consistently when command line arguments --verbose (or -v),
--
2.23.0.windows.1.21.g947f504ebe8.dirty

I'll revise the text with this clarifying information (rephrased to man 
page style)


Philip

Re: [PATCH v4 01/17] sparse-checkout: create builtin with 'list' subcommand

[Elijah Newren]

On Tue, Oct 15, 2019 at 6:56 AM Derrick Stolee via GitGitGadget
 wrote:
> +DESCRIPTION
> +---
> +
> +Initialize and modify the sparse-checkout configuration, which reduces
> +the checkout to a set of directories given by a list of prefixes.
> +
> +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.

I think the wording needs to be a bit more detailed; you copied the
wording from git-switch.txt, but usage of git-switch is not expected
to modify the behavior of other commands.  sparse-checkout, by
contrast, is designed to affect other commands: at the very least
checkout & switch, and likely will affect grep, diff, log, and a host
of others.  Perhaps something like:

THIS COMMAND IS EXPERIMENTAL.  ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
THE FUTURE.

Re: Git Gui: Branch->create currently fails...

[Pratyush Yadav]

On 14/10/19 11:11PM, Philip Oakley wrote:
> On 14/10/2019 18:57, Pratyush Yadav wrote:
> > > list "refs/heads/MSVC-README" [list "commit"
> > > "056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"]
> > > [reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]]
> > > "compat/vcSegmentation fault
> > > 
> > > 
> > > Not exactly the same, but almost. Ends the same place, with as similar 
> > > short
> > > line.
> > > This is run inside the bash that is started directly by the 
> > > git-for-windows
> > > sdk start icon. (Target: C:\git-sdk-64\git-bash.exe   Stat in:
> > > C:/git-sdk-64/)
> > > 
> > > so looks to be MSYS2/bash related.
> > Ah, so it is an upstream issue. I guess we can just report it and wait
> > for them to fix it.
> I'd missed the final 'Segmentation fault' on the last line in the bash
> output that wasn't there for the captured file.
> 
> That was repeatable in re-testing.
> But failed if I changed the $fmt string to a plain text 500 char string
> ("1234567890123...").
> 
> I've still to trim down the complicated $fmt string to see if I can see
> where that seg fault starts (i.e. some form of MVCE), so that it can be
> investigated.
> Possibly should check if the --tcl flag actually invokes any tcl! Otherwise
> it's fully in the Git/G-f-W zone.

A quick look tells me '--tcl' does not invoke any Tcl. It is just used 
to output properly formatted strings for Tcl. The option sets the value 
of 'format.quote_style' in for-each-ref (builtin/for-each-ref.c:33). 
That value is later indirectly ends up being used in the function 
ref_filter.c::quote_formatting.

The Tcl code we execute comes from the long $fmt string, which is built 
in git-gui/choose_rev.tcl:133-147. `for-each-ref` just fills in the 
placeholder values, properly formatting them for use in Tcl.

As an experiment, you can try removing '--tcl' from the `for-each-ref` 
command that segfaults, just to be sure. It would probably output 
invalid Tcl, but since we don't do anything with that output, it doesn't 
really matter, and would let us know if '--tcl' is really the culprit.

> ...
> Just rebuilt (I hope) the Windows Subsystem for Linux (WSL) with git v2.23.0
> installed and got:
> 
> list "refs/heads/MSVC-README" [list "commit"
> "056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"]
> [reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]]
> "compat/vcbuild/README: clean/update 'vcpkg' env for Visual Studio updates"]
> [list "" "" "" [reformat_date ""] ""]
> munmap_chunk(): invalid pointer

A quick Google search tells me munmap_chunk() is probably related to an 
invalid pointer being freed. Either a double free or a free on a pointer 
not allocated by malloc or something similar.

> Aborted (core dumped)
> root@Philip-Win10:/mnt/c/git-sdk-64/usr/src/git#
> 
> 
> That said, haven't got the gitk and git gui to work yet on the WSL because
> it doesn't like the tcl/tk.
> 
> It's a bit of a hole digging exercise.

-- 
Regards,
Pratyush Yadav

Re: [RFC PATCH v1] t/README: the test repo does not have global or system configs

[SZEDER Gábor]

On Wed, Oct 16, 2019 at 01:45:15PM +0100, Philip Oakley wrote:
> Signed-off-by: Philip Oakley 
> ---
> 
> While tring to get to grips with some Git-for-Windows config settings
> for testing >4GiB files, I couldn't find any note in the readme about
> the test system config file sources. 

The path of the system config file is determined at compile time, with
no way to override it at runtime.  Since we don't want external config
files influencing our tests, the only choice we have is to ignore the
system config file; that's why our test framework sets
GIT_CONFIG_NOSYSTEM=1.

> Is this the right place for the information, is it complete enough,
> and is the default config template special?
> 
>  t/README | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/t/README b/t/README
> index 60d5b77bcc..3daa1fa182 100644
> --- a/t/README
> +++ b/t/README
> @@ -485,6 +485,9 @@ This test harness library does the following things:
> the --root option documented above, and a '.stress-' suffix
> appended by the --stress option.
>  
> + - The --global and --system config files are ignored, and
> +   a basic --local config is created in the tst repository.

s/tst/test/

However, note that the global config file isn't really ignored, but
different.  The path of the global config file depends on the values
of the env variables $XDG_CONFIG_HOME and $HOME, and, again, to avoid
external influences, our test framework unsets the former, and
overrides the latter with HOME="$TRASH_DIRECTORY".  IOW the global
config file in our tests is '.../t/trash directory.t1234-foo/.gitconfig'.

> +
>   - Defines standard test helper functions for your scripts to
> use.  These functions are designed to make all scripts behave
> consistently when command line arguments --verbose (or -v),
> -- 
> 2.23.0.windows.1.21.g947f504ebe8.dirty
> 

Re: email as a bona fide git transport

[Santiago Torres Arias]

Hi Willy, Vegard.

On Wed, Oct 16, 2019 at 01:10:09PM +0200, Willy Tarreau wrote:
> Hi Vegard,
> 
> On Wed, Oct 16, 2019 at 12:22:54PM +0200, Vegard Nossum wrote:
> > (cross-posted to git, LKML, and the kernel workflows mailing lists.)
> > 
> > Hi all,
> > 
> > I've been following Konstantin Ryabitsev's quest for better development
> > and communication tools for the kernel [1][2][3], and I would like to
> > propose a relatively straightforward idea which I think could bring a
> > lot to the table.
> > 
> > Step 1:
> > 
> > * git send-email needs to include parent SHA1s and generally all the
> >   information needed to perfectly recreate the commit when applied so
> >   that all the SHA1s remain the same
> > 
> > * git am (or an alternative command) needs to recreate the commit
> >   perfectly when applied, including applying it to the correct parent
> > 
> > Having these two will allow a perfect mapping between email and git;
> > essentially email just becomes a transport for git. There are a lot of
> > advantages to this, particularly that you have a stable way to refer to
> > a patch or commit (despite it appearing on a mailing list), and there
> > is no need for "changeset IDs" or whatever, since you can just use the
> > git SHA1 which is unique, unambiguous, and stable.

I wonder if it'd be also possible to then embed gpg signatures over
send-mail payloads so as they can be transparently transferred to the
commit.

-Santiago


signature.asc
Description: PGP signature

Re: [PATCH 1/1] Improve Japanese translation

[Pratyush Yadav]

On 15/10/19 10:59AM, Junio C Hamano wrote:
> "kdnakt via GitGitGadget"  writes:
> 
> > @@ -661,7 +662,7 @@ msgstr ""
> >  #: lib/merge.tcl:108
> >  #, tcl-format
> >  msgid "%s of %s"
> > -msgstr "%s の %s ブランチ"
> > +msgstr "%2$s の %1$s ブランチ"
> >  
> >  #: lib/merge.tcl:122
> >  #, tcl-format
> > @@ -956,7 +957,7 @@ msgstr "エラー: コマンドが失敗しました"
> >  #: lib/checkout_op.tcl:85
> >  #, tcl-format
> >  msgid "Fetching %s from %s"
> > -msgstr "%s から %s をフェッチしています"
> > +msgstr "%2$s から %1$s をフェッチしています"
> 
> Both of these changes to word order make sense.
> 
> It's a bit surprising that these haven't been noticed/fixed for
> quite some time ;-).

Thanks for the review.

-- 
Regards,
Pratyush Yadav

Re: email as a bona fide git transport

[Pratyush Yadav]

ous versions of a patchset by
>   SHA1 rather than archive links. I propose a new changelog tag for
>   this, maybe "Previous:" or maybe even a full list of "v1:", "v2:",
>   etc. with a SHA1 or ref. Note that these SHA1s do *not* need to exist
>   in Linus's repo, but those who want can pull those branches from the
>   bot-maintained repo on git.kernel.org.
> 
> Advantages:
> 
> - we can keep using email to post patches/patchsets
> 
> - the process is opt-in (but should be encouraged) for both authors and
>   maintainers, and the transition can happen over time
> 
> - there is a central repo for convenience, but it is not necessary for
>   development to happen and is not a single point of failure -- it's
>   more like Linus's repo and can be moved or even replicated from
>   scratch by somebody else simply by having mailing list archives
> 
> - allows quick lookup of patch/patchset <-> email discussion within git
> 
> - allows diffing between versions of a single logical patchset
> 
> - patchset descriptions naturally become part of the changelog that ends
>   up in Linus's tree
> 
> Disadvantages:
> 
> - requires patching git
> 
> - requires a bot to continuously create branches for patchsets sent to
>   mailing lists
> 
> - increased storage/bandwidth for git.kernel.org (?)
> 
> - may need a couple of new wrapper scripts to automate patchset
>   construction/versioning

Just to play the devil's advocate, even though I'm in favor of something 
like this, I'll add in another disadvantage:

- The maintainer can't make small edits before pushing the changes out. 

I do that every now and then for git-gui, and Junio does that sometimes 
for Git. I don't know if the folks over at Linux do something like this, 
but using '--exact' would mean that contributors would have to send a 
re-roll for even minor changes. Its mostly an inconvenience instead of a 
problem, but I thought I'd point it out.
 
> Thoughts?

One more question, not strictly related to your proposal: right now, 
when I apply patches from contributors, I pass '-s' to 'am', so the 
applied commit would have my sign-off. The way I see it, that sign-off 
is supposed to signify that I have the right to push out the commit to 
the "main" repo, just like the author's sign-off means that they have 
the right to send me that commit.

Looking at git.git, I notice that Junio does the same. The new '--exact' 
would be incompatible with '-s', correct (since the commit message has 
changed, the SHA1 would also change)? So firstly, make sure you account 
for something like that if you haven't already (I haven't found the time 
to read your patches yet). Secondly, is it all right for the maintainer 
to just not sign-off on the commits they push out?

-- 
Regards,
Pratyush Yadav

Re: email as a bona fide git transport

[Willy Tarreau]

Hi Vegard,

On Wed, Oct 16, 2019 at 12:22:54PM +0200, Vegard Nossum wrote:
> (cross-posted to git, LKML, and the kernel workflows mailing lists.)
> 
> Hi all,
> 
> I've been following Konstantin Ryabitsev's quest for better development
> and communication tools for the kernel [1][2][3], and I would like to
> propose a relatively straightforward idea which I think could bring a
> lot to the table.
> 
> Step 1:
> 
> * git send-email needs to include parent SHA1s and generally all the
>   information needed to perfectly recreate the commit when applied so
>   that all the SHA1s remain the same
> 
> * git am (or an alternative command) needs to recreate the commit
>   perfectly when applied, including applying it to the correct parent
> 
> Having these two will allow a perfect mapping between email and git;
> essentially email just becomes a transport for git. There are a lot of
> advantages to this, particularly that you have a stable way to refer to
> a patch or commit (despite it appearing on a mailing list), and there
> is no need for "changeset IDs" or whatever, since you can just use the
> git SHA1 which is unique, unambiguous, and stable.

I agree this would be great and have been missing this a number of times,
eventhough I'm aware of git-send-pack/git-receive-pack. The text format
is way more convenient for a lot of reasons. It could also help with
Greg's idea of using the commit IDs to reference bugs, as such IDs could
remain stable within a series before it is merged, and as such referenced
in subsequent commit messages. It could also be useful to avoid losing
notes related to a patch once it's merged.

> Step 3:
> 
> * Instead of describing a patchset in a separate introduction email, we
>   can create a merge commit between the parent of the first commit in
>   the series and the last and put the patchset description in the merge
>   commit [5]. This means the patchset description also gets to be part
>   of git history.
> 
>   (This would require support for git send-email/am to be able to send
>   and apply merge commits -- at least those which have the same tree as
>   one of the parents. This is _not_ yet supported in my proposed git
>   patches.)

That's a good idea, as we've all seen long series with a very detailed
description in patch 0 and much less context in subsequent patches, thus
losing the context once merged.

> * stable SHA1s means we can refer to previous versions of a patchset by
>   SHA1 rather than archive links. I propose a new changelog tag for
>   this, maybe "Previous:" or maybe even a full list of "v1:", "v2:",
>   etc. with a SHA1 or ref. Note that these SHA1s do *not* need to exist
>   in Linus's repo, but those who want can pull those branches from the
>   bot-maintained repo on git.kernel.org.

For me this mainly brings the benefit of finally having a unique identifier
for multiple iterations of a patchset. It then becomes easier to use this
identifier to designate the functional work, regardless of the number of
updates it gets. Of course it's never that black and white since such work
may itself merge multiple other patchsets but for most use cases it can
help.

Willy

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[SZEDER Gábor]

On Wed, Oct 16, 2019 at 07:07:17AM +, Johannes Schindelin via GitGitGadget 
wrote:
> From: Johannes Schindelin 
> 
> Ever since worktrees were introduced, the `git_path()` function _really_
> needed to be called e.g. to get at the `index`. However, the wrong path
> is returned for `index.lock`.

Could you give an example where it returns the wrong path for
'index.lock'?  I tried to reproduce this issue in a working tree, but
no matter what I've tried, 'git rev-parse --git-dir index.lock' always
returned the right path.

> This does not matter as long as the Git executable is doing the asking,
> as the path for that `index.lock` file is constructed from
> `git_path("index")` by appending the `.lock` suffix.
> 
> However, Git GUI just learned to use `--git-path` instead of appending
> relative paths to what `git rev-parse --git-dir` returns (and as a
> consequence not only using the correct hooks directory, but also using
> the correct paths in worktrees other than the main one). And one of the
> paths it is looking for is... you guessed it... `index.lock`.
> 
> So let's make that work as script writers would expect it to.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  path.c   |  4 ++--
>  t/t1500-rev-parse.sh | 15 +++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/path.c b/path.c
> index e3da1f3c4e..ff85692b45 100644
> --- a/path.c
> +++ b/path.c
> @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, 
> match_fn fn,
>   int result;
>   struct trie *child;
>  
> - if (!*key) {
> + if (!*key || !strcmp(key, ".lock")) {
>   /* we have reached the end of the key */
>   if (root->value && !root->len)
>   return fn(key, root->value, baton);
> @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, 
> match_fn fn,
>  
>   /* Matched the entire compressed section */
>   key += i;
> - if (!*key)
> + if (!*key || !strcmp(key, ".lock"))
>   /* End of key */
>   return fn(key, root->value, baton);
>  
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 01abee533d..d318a1eeef 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'git-path in worktree' '
> + test_tick &&
> + git commit --allow-empty -m empty &&
> + git worktree add --detach wt &&
> + test_write_lines >expect \
> + "$(pwd)/.git/worktrees/wt/logs/HEAD" \
> + "$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
> + "$(pwd)/.git/worktrees/wt/index" \
> + "$(pwd)/.git/worktrees/wt/index.lock" &&
> + git -C wt rev-parse >actual \
> + --git-path logs/HEAD --git-path logs/HEAD.lock \
> + --git-path index --git-path index.lock &&
> + test_cmp expect actual

Without the fix applied this test fails with:

  + test_cmp expect actual
  --- expect  2019-10-16 10:20:31.047229423 +
  +++ actual  2019-10-16 10:20:31.051229519 +
  @@ -1,4 +1,4 @@
   /home/szeder/src/git/t/trash 
directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD
  -/home/szeder/src/git/t/trash 
directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD.lock
  +/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/logs/HEAD.lock
   /home/szeder/src/git/t/trash 
directory.t1500-rev-parse/.git/worktrees/wt/index
   /home/szeder/src/git/t/trash 
directory.t1500-rev-parse/.git/worktrees/wt/index.lock
  error: last command exited with $?=1

So the path of 'index.lock' seems to be fine already, it's the path of
the lockfile for HEAD's reflog that's indeed wrong and makes the test
fail.

On a related note, I'm not sure whether the path of the reflogs
directory is right while in a different working tree...  Both with and
without this patch I get a path pointing to the main working tree:

  $ ./git -C WT/ rev-parse --git-path logs
  /home/szeder/src/git/.git/logs

However, I'm not sure what the right path should be in the first
place, given that each working tree has its own 'logs' directory, but
only for HEAD's reflog, while everything else goes to the main working
tree's 'logs' directory.

> +'
> +
>  test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
>   test_commit test_commit &&
>   echo true >expect &&
> -- 
> gitgitgadget

Re: [PATCH v2] Doc: Bundle file usage

[Philip Oakley]

On 16/10/2019 10:57, Philip Oakley wrote:

From: Philip Oakley 
Oops - the From: line still has my old email address. Is a resend 
preferred, or can it be fixed locally?


P.

Git URLs can accept bundle files for fetch, pull and clone, include
in that section. Include git clone in the bundle usage description.
Correct the quoting of .
Detail the  options for cloning a complete repo.

Signed-off-by: Philip Oakley 
---

This takes up the advice from peff in
https://public-inbox.org/git/20191011161112.ga19...@sigill.intra.peff.net/
from the original v1 in 2012(!)
https://public-inbox.org/git/1348010734-664-2-git-send-email-philipoak...@iee.org/

Hopefully this covers Junio's concerns from that time.

Re: Adding a line after the signed-off git am -s

[Daniel Lezcano]

On 16/10/2019 00:52, Beat Bolli wrote:
> On 11.10.19 16:43, Daniel Lezcano wrote:
>>
>> Hi all,
>>
>> Is there a way to specify a line to be added in the change-log after the
>> SOB with git-am ?
>>
>> I would like to do something:
>>
>> git am -s -l "Link: https://lore.kernel.org/r/"
>>
>> Which will give:
>>
>> blabla
>>
>> Signed-off-by: aut...@kairnail.org
>> Signed-off-by: commi...@kairnail.org
>> Link: https://lore.kernel.org/r/
>>
>> This way it is compatible with patchwork, git-pw, etc...
> 
> I think something like
> 
> git interpret-trailer --trailer Link:https://lore.kernel.irg/r/msgid
>  
> should work.

Thank you for the suggestion, I will try.

  -- Daniel


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: What's cooking in git.git (Oct 2019, #04; Tue, 15)

[Elijah Newren]

On Tue, Oct 15, 2019 at 6:25 PM Junio C Hamano  wrote:
>
> Elijah Newren  writes:
>
> >> * en/merge-recursive-directory-rename-fixes (2019-10-12) 2 commits
> >>   (merged to 'next' on 2019-10-15 at ebfdc3ff7b)
> >>  + merge-recursive: fix merging a subdirectory into the root directory
> >>  + merge-recursive: clean up get_renamed_dir_portion()
> >>
> >>  A few glitches in the heuristic in merge-recursive to infer file
> >>  movements based on movements of other files in the same directory
> >>  have been corrected.
> >>
> >>  Will merge to 'master'.
> >
> > I'm surprised this one was merged straight down to next; perhaps I
> > should have highlighted my plans a bit clearer in the thread?
>
> My mistake.  I am willing to revert the merge to give the topic a
> clean slate.  Just tell me so.

Yeah, let's revert it.

> > Also, a very minor point but "glitches" may be misleading; it suggests
> > (to me at least) a malfunction rather than a failure to trigger,...
>
> I used the word to mean a failure to trigger (after all, a heuristic
> that fails to trigger when most people would naturelly expect it to
> is showing a glitch in that case).  A better phrasing, please?

Oh, I guess I just had a different connotation for glitch.  I guess
what you had is fine then, but alternatively we could spell it out
just a little more:

When all files from some subdirectory were renamed to the root
directory, the directory rename heuristics would fail to detect that
as a rename/merge of the subdirectory to the root directory, which has
been corrected.

Re: [PATCH 1/1] notes: copy notes to HEAD by default

[Danh Doan]

On 2019-10-16 11:01:34 +0900, Junio C Hamano wrote:
> Doan Tran Cong Danh  writes:
> 
> > The target objects for copying notes was defaulted to HEAD from very
> > early stage of git-notes.
> >
> > However, that default was limited by commit bbb1b8a35a, ("notes: check
> > number of parameters to "git notes copy"", 2010-06-28).
> 
> Sorry, I don't quite get the above.  The said commit made sure 'git
> notes copy' gets the right number of arguments, saying """Otherwise
> we may segfault with too few parameters."""  I take that as a sign
> that before that commit it was not defaulting to HEAD but attempting
> to access the missing argv[2] (or whatever the index the 
> should be at) and dereferencing a NULL?
> 
> ... goes and digs ...
> 
> I think v1.6.6.1-458-g74884b524e is the commit that made the command
> line parsing into the current shape, i.e. one parse_options() call
> in each of the subcommand that gets dispatched, and you are right
> that with that version a single argument given on the command line
> is taken as the  and  defaults to HEAD.
> 
> So... what happend between that vesrion and v1.7.1-200-gbbb1b8a35a?
> 
> ... goes and looks at bbb1b8a35a again ...
> 
> Ah, I think there is an off-by-one.  When not from-stdin and not
> using rewrite-cmd, before that patch, we did not even check if
> from-obj exists, so in that sense, the commit had a right idea that
> it must check for "too few parameters", but it shouldn't have
> insisted that we have at least two.  It is OK to have just one,
> i.e. only the from-obj, for our purpose.

Yes, this is my intention.

> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index d3fa298c6a..a8f9a0f36c 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes 
> > copy -f"' '
> > git notes copy -f HEAD~2 HEAD &&
> > git log -1 >actual &&
> > test_cmp expect actual &&
> > +   test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
> > +   git notes copy -f HEAD~2 &&
> > +   git log -1 >actual &&
> > +   test_cmp expect actual &&
> > test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
> >  '
> 
> This I am not sure is a good test to add to, especially as a fix to

I was writing this patch just before my bed time, just to get some
comments on the directions, e.g:
- loosen the argc requirements; or
- do the code cleanup

> bbb1b8a, which added this test:
> 
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 64f32ad94d..2d67a40fc1 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1044,4 +1044,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides 
> config' '
>   git log -1 > output &&
>   test_cmp expect output
>  '
> +
> +test_expect_success 'git notes copy diagnoses too many or too few 
> parameters' '
> + test_must_fail git notes copy &&
> + test_must_fail git notes copy one two three
> +'
> +
>  test_done
> 
> The lack of testing that "git notes copy " succeeding is
> why the off-by-one bug was not noticed, so I think that test (which
> still exists to this day) is the right place to add a test to
> protect this fix.

I don't think this is a good place to add this test either,
since the test description specificaly said it diagnoses too many or
too few parameters.

Anyway, the test `git notes copy one two three` still fails if we
accidentally allow 3 arguments since one two three isn't valid ref.
I'm gonna add a statement to assert the diagnose message.

Since I don't want to update the commit id (e.g. 10th, 11th, etc..)
of other test cases, I think it'd be better to modify the current test:

- for the test case without '-f' flag, remove HEAD if it's the target,
  and add a test-case to copy to somewhere else. Well, all of our current
  test cases only test with HEAD as target-object.
- for the test case with '-f' flag, I think I'll keep my current
  approach.

> 
> As to the log message, here is how I would explain/justify the
> change, if I were writing it.

I'll update the commit message.

-- 
Danh

Re: [PATCH v3 01/13] graph: automatically track display width of graph lines

[Junio C Hamano]

Junio C Hamano  writes:

>>  int graph_next_line(struct git_graph *graph, struct strbuf *sb)
>
> I just noticed it but does this have to be extern?  Nobody outside
> graph.[ch] seems to have any reference to it.

I was stupid; strike this part out.

Thanks.

Re: [RFC PATCH 1/1] Teach remote add the --prefix-tags option

[Jacob Keller]

On Tue, Oct 15, 2019 at 2:13 PM Wink Saville  wrote:
>>
>> Something like this makes sense and I've thought about the problem for
>> a long time. Unfortunately it's quite a bit trickier to do this.
>>
>> It would solve the problem more generally though, and definitely seems
>> like the right approach.. but at least for me, every time I looked at
>> trying this I got lost. I haven't had time to investigate it recently
>> :(
>>
>> Thanks,
>> Jake
>
>
> Give it a go, you'll learn something at a minimum :)

I've started a couple of times, but mostly it's lack of time to
invest, since $DAYJOB hasn't given me cycles to try at the moment.

Thanks,
Jake

Re: [PATCH v3 08/13] graph: tidy up display of left-skewed merges

[Junio C Hamano]

"James Coglan via GitGitGadget"  writes:

> This effect is applied to both "normal" two-parent merges, and to
> octopus merges. It also reduces the vertical space needed for pre-commit
> lines, as the merge occupies one less column than usual.
>
> Before: After:
>
> | * | *
> | |\| |\
> | | \   | * \
> | |  \  |/|\ \
> | *-. \
> | |\ \ \

Looking at these drawings reminded me of a tangent that is brought
up from time to time.  We do not do great job when showing multiple
roots.

If you have a history like this:

  A---D---E
 /
B---C

drawing the graph _with_ the merge gives a reasonable representation
of the entire topology.

* 46f67dd E
*   6f89516 D
|\  
| * e6277a9 C
| * 13ae9b2 B
* afee005 A

But if you start drawing from parents of D (excluding D), you'd get
this:

* e6277a9 C
* 13ae9b2 B
* afee005 A

and the fact that B and A do not share parent-child relationships is
lost.  An easy way to show that would be to draw the bottom three
lines of the full history output we saw earlier:

| * e6277a9 C
| * 13ae9b2 B
* afee005 A

either with or without the vertical bar to imply that A may have a
child.

This is not something that has to be done as part of this series,
but I am hoping that the internal simplification and code
restructuring that is done by this series would make it easier to
enhance the system to allow such an output.

Thanks.

Re: [PATCH v3 02/13] graph: handle line padding in `graph_next_line()`

[Junio C Hamano]

"James Coglan via GitGitGadget"  writes:

> From: James Coglan 
>
> Now that the display width of graph lines is implicitly tracked via the
> `graph_line` interface, the calls to `graph_pad_horizontally()` no
> longer need to be located inside the individual output functions, where
> the character counting was previously being done.
>
> All the functions called by `graph_next_line()` generate a line of
> output, then call `graph_pad_horizontally()`, and finally change the
> graph state if necessary. As padding is the final change to the output
> done by all these functions, it can be removed from all of them and done
> in `graph_next_line()` instead.

Very well explained.

Re: [PATCH v3 01/13] graph: automatically track display width of graph lines

[Junio C Hamano]

"James Coglan via GitGitGadget"  writes:

> +struct graph_line {
> + struct strbuf *buf;
> + size_t width;
> +};
> +
> +static inline void graph_line_addch(struct graph_line *line, int c)
> +{
> + strbuf_addch(line->buf, c);
> + line->width++;
> +}
> +
> +static inline void graph_line_addchars(struct graph_line *line, int c, 
> size_t n)
> +{
> + strbuf_addchars(line->buf, c, n);
> + line->width += n;
> +}
> +
> +static inline void graph_line_addstr(struct graph_line *line, const char *s)
> +{
> + strbuf_addstr(line->buf, s);
> + line->width += strlen(s);
> +}
> +
> +static inline void graph_line_addcolor(struct graph_line *line, unsigned 
> short color)
> +{
> + strbuf_addstr(line->buf, column_get_color_code(color));
> +}

All makes sense, and as long as nobody uses strbuf_add*() on
line->buf directly, it shouldn't be too hard to extend these
to support graph drawn characters outside ASCII in the future
after this series settles.

>  static void graph_output_pre_commit_line(struct git_graph *graph,
> ...
>   for (i = 0; i < graph->num_columns; i++) {
>   struct column *col = &graph->columns[i];
>   if (col->commit == graph->commit) {
>   seen_this = 1;
> - strbuf_write_column(sb, col, '|');
> - strbuf_addchars(sb, ' ', graph->expansion_row);
> - chars_written += 1 + graph->expansion_row;
> + graph_line_write_column(line, col, '|');
> + graph_line_addchars(line, ' ', graph->expansion_row);

Nice reduction of noise, as the proposed log message promises.

>  int graph_next_line(struct git_graph *graph, struct strbuf *sb)

I just noticed it but does this have to be extern?  Nobody outside
graph.[ch] seems to have any reference to it.  That might mean that
it may be easier than we thought earlier to change the second
parameter of this to "struct graph_line *", instead of us wrapping
an incoming strbuf (which external callers are aware of, as opposed
to graph_line which we prefer not to expose to outsiders).  Whether
we change the second parameter or not, the first clean-up may be to
turn this function into a file-scope static, perhaps?

All the changes are very pleasing to see.  Thanks.

Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`

[Junio C Hamano]

James Coglan  writes:

>> Is there a reason why you need a pointer to a strbuf that is
>> allocated separately?  E.g. would it make it harder to manage
>> if the above were
>> 
>>  struct graphbuf {
>>  struct strbuf buf;
>>  int width; /* display width in columns */
>>  };
>> 
>> which is essentially what Dscho suggested?
>
> I used a pointer here because I create the wrapper struct in
> `graph_next_line()`, which is an external interface that takes a
> `struct strbuf *`:
>
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> {
>   struct graph_line line = { .buf = sb, .width = 0 };
>   // ...
> }
>
> So I'm not allocating the strbuf here, just wrapping a pointer to
> it.

OK, so existing callers allocate strbuf, and you are merely adding a
wrapper structure to keep track of the width.  The management of the
lifetime of the strbuf is not your business so there is no reason to
inline the structure in graph_line.  Makes sense.  Thanks.

Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag

[Junio C Hamano]

William Baker  writes:

> At this time there are no other MIDX_* flags and so there's always the option
> to revisit the best way to handle multiple MIDX_* flags if/when additional
> flags are added.

I do not care too deeply either way, but if you wrote it in one way,
how about completing the series without changing it in the middle,
and leave the clean-ups to a follow-up series (if needed)?

Re: [PATCH v3 1/1] doc: Change zsh git completion file name

[Junio C Hamano]

"Maxim Belsky via GitGitGadget"  writes:

> From: Maxim Belsky 
> Subject: Re: [PATCH v3 1/1] doc: Change zsh git completion file name

Lack of attention to the detail.  This is not about changing the
filename anymore.

Subject: [PATCH v3] completion: clarify installation instruction for zsh

perhaps.  I'll make the change locally, so unless there are other
changes you want to make, there is no need to resend.

Thanks.

> The original comment does not describe type of ~/.zsh/_git explicitly
> and zsh does not warn or fail if a user create it as a dictionary.
> So unexperienced users could be misled by the original comment.
>
> There is a small update to clarify it.
>
> Helped-by: Johannes Schindelin 
> Helped-by: Junio C Hamano 
> Helped-by: SZEDER Gábor 
> Signed-off-by: Maxim Belsky 
> ---
>  contrib/completion/git-completion.zsh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.zsh 
> b/contrib/completion/git-completion.zsh
> index 886bf95d1f..b480e3f316 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -11,8 +11,9 @@
>  #
>  #  zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh
>  #
> -# The recommended way to install this script is to copy to '~/.zsh/_git', and
> -# then add the following to your ~/.zshrc file:
> +# The recommended way to install this script is to make a copy of it in
> +# '~/.zsh/' directory as '~/.zsh/_git' and then add the following to your
> +# ~/.zshrc file:
>  #
>  #  fpath=(~/.zsh $fpath)