Re: [PATCH v5 6/6] add worktree.guessRemote config option

[Junio C Hamano]

Thomas Gummerer  writes:

> +worktree.guessRemote::
> + With `add`, if no branch argument, and neither of `-b` nor
> + `-B` nor `--detach` are given, the command defaults to
> + creating a new branch from HEAD.  If `worktree.guessRemote` is
> + set to true, `worktree add` tries to find a remote-tracking
> + branch whose name uniquely matches the new branch name.  If
> + such a branch exists, it is checked out and set as "upstream"
> + for the new branch.  If no such match can be found, it falls
> + back to creating a new branch from the current HEAD.

Unlike the part I commented on in the previous step, this one is
clear that the feature only kicks in for 'add ' without
anything else, which is good.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 15cb1600ee..426aea8761 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -33,8 +33,19 @@ struct add_opts {
>  
>  static int show_only;
>  static int verbose;
> +static int guess_remote;
>  static timestamp_t expire;
>  
> +static int git_worktree_config(const char *var, const char *value, void *cb)
> +{
> + if (!strcmp(var, "worktree.guessremote")) {
> + guess_remote = git_config_bool(var, value);
> + return 0;
> + }
> +
> + return 0;
> +}
> +

It is a lot more consistent with the established practice if this
function had

return git_default_config(var, value, cb);

instead of "return 0" at the end, and then have the call to

git_config(git_default_config, NULL);

we have in cmd_worktree() replaced with

git_config(git_worktree_config, NULL);

That would avoid having to scan the entire set of config keys once
in cmd_worktree() and then again in add(), the latter one only
looking for a single variable.

Re: [PATCH v5 5/6] worktree: add --guess-remote flag to add subcommand

[Junio C Hamano]

Thomas Gummerer  writes:

> Currently 'git worktree add ' creates a new branch named after the
> basename of the , that matches the HEAD of whichever worktree we
> were on when calling "git worktree add ".
>
> It's sometimes useful to have 'git worktree add  behave more like
> the dwim machinery in 'git checkout ', i.e. check if the new
> branch name uniquely matches the branch name of a remote-tracking
> branch, and if so check out that branch and set the upstream to the
> remote-tracking branch.

This paragraph was a bit hard to sympathize because it was not
obvious that the new feature still assumes how  is used to
compute the name of the new branch.  Perhaps if it were written like
so:

check if the new branch name, derived from the basename of
the , uniquely matches the branch name of ...

I would not have had to read it twice to understand what was going
on.

> +--[no-]guess-remote::
> + With `add`, instead of creating a new branch from HEAD when
> + `` is not given, if there exists a tracking branch
> + in exactly one remote matching the basename of the path, base
> + the new branch on the remote-tracking branch, and mark the
> + remote-tracking branch as "upstream" from the new branch.
> +

Would

git worktree add --guess-remote  

be an error?  It is allowed as long as  and the basename of
the  matches?  The option is silently ignored?  Something
else?

I am reacting to "with `add`" part of this desciption.  I wouldn't
be asking if it said "With `worktree add ` without ",
as that would make the scenario I am wondering about automatically
"undefined".  Yes, we should strive for leaving things undefined as
little as practically possible, but at least saying something like
"without " explicitly there would make sure that readers
know in what scenario this option is meant to be used a bit better.

> @@ -389,6 +392,13 @@ static int add(int ac, const char **av, const char 
> *prefix)
>   int n;
>   const char *s = worktree_basename(path, );
>   opts.new_branch = xstrndup(s, n);
> + if (guess_remote) {
> + struct object_id oid;
> + const char *remote =
> + unique_tracking_name(opts.new_branch, );
> + if (remote)
> + branch = remote;
> + }
>   }

I think the answer is "silently ignored", as the above hunk is
inside "if (ac < 2 && !opts.new_branch && !opts.detach)".

Re: git status always modifies index?

[Jeff King]

On Mon, Nov 27, 2017 at 09:47:20AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I'm not sure I agree. Lockless writes are actually fine for the original
> > use case of --no-optional-locks (which is a process for the same user
> > that just happens to run in the background).
> 
> The phrase "lockless write" scares me---it sounds as if you
> overwrite the index file no matter what other people (including
> another instance of yourself) are doing to it.  

Ick, no, that would be quite bad. ;)

I only meant that if we "somehow" had a way in the future to update the
stat cache without affecting the other parts of the index, and without
causing lock contention that causes other readers to barf, it could be
triggered even under this option.

That would be quite different from the current index and stat-cache
design, and I have no plans in that area.

Writes to the object database _are_ lockless now (it is OK if two
writers collide, because they are by definition writing the same data).
And I wouldn't expect them to be affected by --no-optional-locks.  I
think elsewhere in the thread you mentioned writing out trees for
cache-tree, which seems like a plausible example. Usually there's not
much point if you're not going to write out the index with the new
cache-tree entries, too. But I could see a program wanting to convert
the index into a tree in order to speed up a series of tree-to-index
diffs within a single program.

This is all pretty hypothetical, though.

-Peff

[PATCH] git-status.txt: mention --no-optional-locks

[Jeff King]

On Mon, Nov 27, 2017 at 12:24:43AM -0500, Jeff King wrote:

> > If people have to ask on the mailing list even after reading the man
> > pages, that's a strong indicator that we could do better.
> 
> Sure. That's why I suggested improving the documentation in my last
> email. But in all the discussion, I haven't seen any patch to that
> effect.

Maybe like this.

-- >8 --
Subject: [PATCH] git-status.txt: mention --no-optional-locks

If you come to the documentation thinking "I do not want Git
to take any locks for my background processes", then you may
easily run across "--no-optional-locks" in git.txt.

But it's quite reasonable to hit a specific instance of the
problem: you have "git status" running in the background,
and you notice that it causes lock contention with other
processes. So you look in git-status.txt to see if there is
a way to disable it, but there's no mention of the flag.

Let's add a short note mentioning that status does indeed
touch the index (and why), with a pointer to the global
option. That can point users in the right direction and help
them make a more informed decision about what they're
disabling.

Signed-off-by: Jeff King 
---
 Documentation/git-status.txt | 13 +
 1 file changed, 13 insertions(+)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index fc282e0a92..81cab9aefb 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -387,6 +387,19 @@ ignored submodules you can either use the 
--ignore-submodules=dirty command
 line option or the 'git submodule summary' command, which shows a similar
 output but does not honor these settings.
 
+BACKGROUND REFRESH
+--
+
+By default, `git status` will automatically refresh the index, updating
+the cached stat information from the working tree and writing out the
+result. Writing out the updated index is an optimization that isn't
+strictly necessary (`status` computes the values for itself, but writing
+them out is just to save subsequent programs from repeating our
+computation). When `status` is run in the background, the lock held
+during the write may conflict with other simultaneous processes, causing
+them to fail. Scripts running `status` in the background should consider
+using `git --no-optional-locks status` (see linkgit:git[1] for details).
+
 SEE ALSO
 
 linkgit:gitignore[5]
-- 
2.15.0.687.g5a800c9f78

Re: [PATCH] git-status.txt: mention --no-optional-locks

[Junio C Hamano]

Jeff King  writes:

> On Mon, Nov 27, 2017 at 12:24:43AM -0500, Jeff King wrote:
>
>> > If people have to ask on the mailing list even after reading the man
>> > pages, that's a strong indicator that we could do better.
>> 
>> Sure. That's why I suggested improving the documentation in my last
>> email. But in all the discussion, I haven't seen any patch to that
>> effect.
>
> Maybe like this.

I gave it only a single read, and it was a quite easy read.

Will queue but not immediately merge to 'next' before I hear from
others.

Thanks.

> -- >8 --
> Subject: [PATCH] git-status.txt: mention --no-optional-locks
>
> If you come to the documentation thinking "I do not want Git
> to take any locks for my background processes", then you may
> easily run across "--no-optional-locks" in git.txt.
>
> But it's quite reasonable to hit a specific instance of the
> problem: you have "git status" running in the background,
> and you notice that it causes lock contention with other
> processes. So you look in git-status.txt to see if there is
> a way to disable it, but there's no mention of the flag.
>
> Let's add a short note mentioning that status does indeed
> touch the index (and why), with a pointer to the global
> option. That can point users in the right direction and help
> them make a more informed decision about what they're
> disabling.
>
> Signed-off-by: Jeff King 
> ---
>  Documentation/git-status.txt | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index fc282e0a92..81cab9aefb 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -387,6 +387,19 @@ ignored submodules you can either use the 
> --ignore-submodules=dirty command
>  line option or the 'git submodule summary' command, which shows a similar
>  output but does not honor these settings.
>  
> +BACKGROUND REFRESH
> +--
> +
> +By default, `git status` will automatically refresh the index, updating
> +the cached stat information from the working tree and writing out the
> +result. Writing out the updated index is an optimization that isn't
> +strictly necessary (`status` computes the values for itself, but writing
> +them out is just to save subsequent programs from repeating our
> +computation). When `status` is run in the background, the lock held
> +during the write may conflict with other simultaneous processes, causing
> +them to fail. Scripts running `status` in the background should consider
> +using `git --no-optional-locks status` (see linkgit:git[1] for details).
> +
>  SEE ALSO
>  
>  linkgit:gitignore[5]

Re: git status always modifies index?

[Junio C Hamano]

Jeff King  writes:

> Again, maybe the bit above explains my viewpoint a bit more. I'm
> certainly sympathetic to the pain of upstreaming.
>
> I do disagree with the "no good reason" for this particular patch.
>
> Certainly you should feel free to present your hunches. I'd expect you
> to as part of the review (I'm pretty sure I even solicited your opinion
> when I sent the original patch). But I also think it's important for
> patches sent upstream to get thorough review (both for code and design).
> The patches having been in another fork (and thus presumably being
> stable) is one point in their favor, but I don't think it should trumps
> all other discussion.

I haven't been following this subthread closely, but I agree.  I
think your turning a narrow option that was only about status into
something that can be extended as a more general option resulted in
a better design overall.

I am guessing that a little voice in his head said "this may be
applicable wider than Windows and it will be better to be humble and
receptive to others' suggestions by going to the list and get this
patch reviewed" was overridden by other needs, like expediency, when
he did the initial "covers only status and its opportunistic writing
of the index" as a Windows only thing, and Dscho is now regretting
not following that initial hunch, as that resulted in unnecessary
pain for both himself and his users.  I am sympathetic, but we are
all normal human and I do not think and mistakes like this can be
avoided.  Often we are blinded by the immediate issue in front of us
and we lose sight of a bigger picture, and it is OK as long as we
learn from our (or better yet, others') mistakes.

Thanks.

Re: [PATCH 5/5] t3404: add test case for abbreviated commands

[Junio C Hamano]

Liam Beguin  writes:

> Make sure the todo list ends up using single-letter command
> abbreviations when the rebase.abbreviateCommands is enabled.
> This configuration options should not change anything else.
>
> Signed-off-by: Liam Beguin 
> ---
>  t/t3404-rebase-interactive.sh | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 6a82d1ed876d..e460ebde3393 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects 
> rebase.missingCommitsCheck = error' '
>   test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_success 'prepare rebase.abbreviateCommands' '
> + reset_rebase &&
> + git checkout -b abbrevcmd master &&
> + test_commit "first" file1.txt "first line" first &&
> + test_commit "second" file1.txt "another line" second &&
> + test_commit "fixup! first" file2.txt "first line again" first_fixup &&
> + test_commit "squash! second" file1.txt "another line here" second_squash
> +'
> +
> +cat >expected < +p $(git rev-list --abbrev-commit -1 first) first
> +f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
> +x git show HEAD
> +p $(git rev-list --abbrev-commit -1 second) second
> +s $(git rev-list --abbrev-commit -1 second_squash) squash! second
> +x git show HEAD
> +EOF

Please have this cat inside and at the beginning of the next
test_expect_success, preferably indented with HT to align with other
commands (adding '-' to the opening of your here-document makes the
shell strip all leading HTs from the here-document), like this:

test_expect_success 'respects rebase' '
cat expect <<-EOF &&
p $(git rev-list ...)
f $(git rev-list ...)
...
EOF
test_when_finished "
...
" &&
...
test_cmp expect actual
'

Re: git status always modifies index?

[Jeff King]

On Sun, Nov 26, 2017 at 10:55:01PM +0100, Johannes Schindelin wrote:

> > I remain unconvinced that we have actually uncovered a serious problem.
> 
> You did not. A colleague of mine did. And it was a problem in Git for
> Windows only, caused by the changes necessitated by yours (which even used
> my tests, which made it easy for my conflict resolution to do the wrong
> thing by removing my --no-lock-index test in favor of your
> --no-optional-locks test, breaking --no-lock-index).
> 
> It cost me almost two work days, and a lot of hair. And all I meant by "I
> hinted at it, too" was that I felt that something like that was coming
> when I saw your variation of my patches making it into git/git's master.

I was confused by your mention of a problem, since this was the first I
heard about it. Looking at the GfW repo, I assume you mean the bits
touched by 45538830baf.

If so, then yes, I'm sad that the combination of the features caused
extra work for you. But I also don't think that is a compelling reason
to say that "--no-optional-locks" is the wrong approach.

It _does_ argue for trying to take features intact between the two
codebases. But I am not sure I buy that argument. The original feature
got no review on the list, and in fact most of us weren't even aware of
it until encountering the problem independently. IMHO it argues for GfW
trying to land patches upstream first, and then having them trickle in
as you merge upstream releases.

I suspect you are going to say "but I am busy and don't have time for
that". And I know it takes time. I maintain the fork that GitHub runs on
its servers, and I have a backlog of features to upstream. Some of them
end up quite different when I do that, and it's a huge pain. But
ultimately I've forked the upstream project, and that's the price I pay.
Upstream doesn't care about my fork's problems.

I dunno. Maybe you do not see Git for Windows as such a fork. But
speaking as somebody who works on git.git, that is my perception of it
(that GfW is downstream). So I'm sympathetic, but I don't like the idea
of taking non-Windows-specific patches wholesale and skipping the list
review and design process.

> > Somebody asked if Git could do a thing, and people pointed him to the
> > right option.
> 
> If people have to ask on the mailing list even after reading the man
> pages, that's a strong indicator that we could do better.

Sure. That's why I suggested improving the documentation in my last
email. But in all the discussion, I haven't seen any patch to that
effect.

> In Git, yes. In Git for Windows, no. And it worked beautifully in Git for
> Windows before v2.15.0.

It didn't in git.git, because it wasn't there. ;)

> > But I figured you would carry "status --no-lock-index" forever in Git
> > for Windows anyway (after all, if you remove it now, you're breaking
> > compatibility for existing users).
> 
> I will not carry it forever. Most definitely not. It was marked as
> experimental for a reason: I suspected that major changes would be
> required to get it accepted into git.git (even if I disagree from a purely
> practicial point of view that those changes are required, but that's what
> you have to accept when working in Open Source, that you sometimes have to
> change something solely to please the person who can reject your patches).

Yes, I saw just now that you continue to recognize it and give a
deprecation warning, which seems like quite a reasonable thing to do.

> Sure, I am breaking compatibility for existing users, but that is more the
> fault of --no-optional-locks being introduced than anything else.

Hopefully the text at the start of this mail explains why I disagree on
the "fault" here.

> I am pretty much done talking about this subject at this point. I only
> started talking about it because I wanted you to understand that I will
> insist on my hunches more forcefully in the future, and I hope you will
> see why I do that. But then, you may not even see the problems caused by
> the renaming (and forced broader scope for currently no good reason) of
> --no-lock-index, so maybe you disagree that acting on my hunch would have
> prevented those problems.

Again, maybe the bit above explains my viewpoint a bit more. I'm
certainly sympathetic to the pain of upstreaming.

I do disagree with the "no good reason" for this particular patch.

Certainly you should feel free to present your hunches. I'd expect you
to as part of the review (I'm pretty sure I even solicited your opinion
when I sent the original patch). But I also think it's important for
patches sent upstream to get thorough review (both for code and design).
The patches having been in another fork (and thus presumably being
stable) is one point in their favor, but I don't think it should trumps
all other discussion.

-Peff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Todd Zullinger]

Jeff King wrote:
Yeah, this side-steps the "other half" of the issue that Christian's 
patch addresses, which seems like the more controversial part (I don't 
have a strong opinion myself, though).


I don't either.  The general motivation there, as far as I understand, 
is that it's undesirable to have 'make install' install tools that 
cannot run.


Perhaps it's worth noting that there are other commands installed by 
default which won't work out of the box.  For example, 'git svn'

requires subversion at run time but not at build time.

If there aren't many such commands, then maybe checking for them in 
the Makefile is reasonable to make installing git from source easier 
for new users.  Without looking closely, I can't do more than take a 
wild guess.


As a package maintainer and an aging sysadmin, I'd bet that there are 
more dependencies than one might initially guess.  I also tend to 
think that by the time most users want something newer than their OS 
provides that they're a bit more able to deal with these sort of 
issues.  But I could easily be wrong on all counts.


--
Todd
~~
Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
   -- Hunter S. Thompson

Re: [PATCH 0/5] rebase -i: add config to abbreviate command names

[Junio C Hamano]

Liam Beguin  writes:

> Liam Beguin (5):
>   Documentation: move rebase.* configs to new file
>   Documentation: use preferred name for the 'todo list' script
>   rebase -i: add exec commands via the rebase--helper
>   rebase -i: learn to abbreviate command names
>   t3404: add test case for abbreviated commands

I didn't send any comment on [1&2/5] but they both looked good.

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[Junio C Hamano]

Liam Beguin  writes:

>   if (command == MAKE_SCRIPT && argc > 1)
> - return !!sequencer_make_script(keep_empty, stdout, argc, argv);
> + return !!sequencer_make_script(keep_empty, abbreviate_commands,
> +stdout, argc, argv);

This suggests that a preliminary clean-up to update the parameter
list of sequencer_make_script() is in order just before this step.
How about making it like so, perhaps:

int sequencer_make_script(FILE *out, int ac, char **av, unsigned flags)

where keep_empty becomes just one bit in that flags word.  Then another
bit in the same flags word can be used for this option.

Otherwise, every time somebody comes up with a new and shiny feature
for the function, we'd end up adding more to its parameter list.

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Junio C Hamano]

Liam Beguin  writes:

> diff --git a/sequencer.c b/sequencer.c
> index fa94ed652d2c..810b7850748e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>   return 0;
>  }
>  
> +int add_exec_commands(const char *command)
> +{

As the name of a public function, it does not feel that this hints
it strongly enough that it is from and a part of sequencer.c API.

> + const char *todo_file = rebase_path_todo();
> + struct todo_list todo_list = TODO_LIST_INIT;
> + int fd, res, i, first = 1;
> + FILE *out;

Having had to scan backwards while trying to see what the loop that
uses this variable is doing and if it gets affected by things that
happened before we entered the loop, I'd rather not to see 'first'
initialized here, left unused for quite some time until the loop is
entered.  It would make it a lot easier to follow if it is declared
and left uninitilized here, and set to 1 immediately before the
for() loop that uses it.

> +
> + strbuf_reset(_list.buf);
> + fd = open(todo_file, O_RDONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s'"), todo_file);
> + if (strbuf_read(_list.buf, fd, 0) < 0) {
> + close(fd);
> + return error(_("could not read '%s'."), todo_file);
> + }
> + close(fd);

Is this strbuf_read_file() written in longhand?

> + res = parse_insn_buffer(todo_list.buf.buf, _list);
> + if (res) {
> + todo_list_release(_list);
> + return error(_("unusable todo list: '%s'"), todo_file);
> + }
> +
> + out = fopen(todo_file, "w");
> + if (!out) {
> + todo_list_release(_list);
> + return error(_("unable to open '%s' for writing"), todo_file);
> + }
> + for (i = 0; i < todo_list.nr; i++) {
> + struct todo_item *item = todo_list.items + i;
> + int bol = item->offset_in_buf;
> + const char *p = todo_list.buf.buf + bol;
> + int eol = i + 1 < todo_list.nr ?
> + todo_list.items[i + 1].offset_in_buf :
> + todo_list.buf.len;

Should bol and eol be of type size_t instead?  The values that get
assigned to them from other structures are.

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Jeff King]

On Mon, Nov 27, 2017 at 01:53:34PM +0900, Junio C Hamano wrote:

> > I'd be curious if the 5th patch there provides an additional speedup for
> > Takuto's case.
> 
> Indeed, it is a very good point.
> 
> IIUC, the 5th one is about fetching tons of refs that you have never
> seen, right?  If a repository that has trouble with everything-local
> is suffering because it right now has 300k remote-tracking branches,
> I'd imagine that these remote-tracking branches are being added at a
> considerable rate, so I'd not be surprised if these "new" refs
> benefits from that patch.  And it would be nice to know how much a
> real life scenario actually does improve.

Right, I think it will only kick in for new refs (and maybe deleted
ones, but I didn't check).

I actually did have a real life scenario that was improved, but it's not
very typical. As I've mentioned before, we keep a big shared-object
repository for forks of a single repo, so a fairly common operation is

  git -C network.git fetch ../$fork.git +refs/*:refs/remotes/$fork/*

The first time that's run on a fork, everything looks like a new ref.

So "real life", but thankfully not life for most git users. :)

-Peff

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Takuto Ikuta]

2017-11-27 13:53 GMT+09:00 Junio C Hamano :
> Jeff King  writes:
>
>>> cf. 
>>> https://public-inbox.org/git/20171120202920.7ppcwmzkxifyw...@sigill.intra.peff.net/
>>
>> It's funny that we'd get two patches so close together. AFAIK the
>> slowness here has been with us for years, and I just happened to
>> investigate it recently.

Yes, thank you for let me know.
Please ignore my patch, sorry.

>>
>>> The 5-patch series that contains the same change as this one is
>>> cooking and will hopefully be in the released version before the end
>>> of the year.
>>
>> I'd be curious if the 5th patch there provides an additional speedup for
>> Takuto's case.
>
> Indeed, it is a very good point.
>
> IIUC, the 5th one is about fetching tons of refs that you have never
> seen, right?  If a repository that has trouble with everything-local
> is suffering because it right now has 300k remote-tracking branches,
> I'd imagine that these remote-tracking branches are being added at a
> considerable rate, so I'd not be surprised if these "new" refs
> benefits from that patch.  And it would be nice to know how much a
> real life scenario actually does improve.
>
> Thanks.

In chromium repository,  your 5th patch does not improve performance,
took more than 5 minutes to run fetch on windows.
4th patch is very important for the repository in daily fetch.
I hope your 4th patch will be merged.

Thanks.
-- 
Takuto Ikuta
Software Engineer in Tokyo
Chrome Infrastructure (goma team)

Re: git status always modifies index?

[Jeff King]

On Mon, Nov 27, 2017 at 01:56:41PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I actually consider "--no-optional-locks" to be such an aspirational
> > feature. I didn't go digging for other cases (though I'm fairly certain
> > that "diff" has one), but hoped to leave it for further bug reports ("I
> > used the option, ran command X, and saw lock contention").
> 
> OK, then we are essentially on the same page.  I just was hoping
> that we can restrain ourselves from adding these "non essential"
> knobs at too fine granularity, ending up forcing end users to use
> all of them.

Yes, I agree we should try not to have too many knobs. That's actually
one of the reasons I avoided a status-only option in the first place.

In retrospect, I agree that the current option probably doesn't get the
granularity quite right. The idea of "totally read-only" just didn't
cross my mind at all when working on the earlier feature.

-Peff

Re: git status always modifies index?

[Junio C Hamano]

Jeff King  writes:

> I actually consider "--no-optional-locks" to be such an aspirational
> feature. I didn't go digging for other cases (though I'm fairly certain
> that "diff" has one), but hoped to leave it for further bug reports ("I
> used the option, ran command X, and saw lock contention").

OK, then we are essentially on the same page.  I just was hoping
that we can restrain ourselves from adding these "non essential"
knobs at too fine granularity, ending up forcing end users to use
all of them.

[PATCH 0/5] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series is a respin of something [1] I sent a few months ago. This
time, instead of shell, It's based on top of the C implementation of the
interactive rebase. I've also tried to address the comments that were
left in the last thread.

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r" instead of
"ciw").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes since last time:
- Implement abbreviateCommands in rebase--helper
- Add note on the --[no-]autosquash option in rebase.autoSquash
- Add exec commands via the rebase--helper
- Add test case for rebase.abbreviateCommands

Liam Beguin (5):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt|  31 +
 Documentation/git-rebase.txt|  19 +---
 Documentation/rebase-config.txt |  51 
 builtin/rebase--helper.c|  17 +--
 git-rebase--interactive.sh  |  23 +
 sequencer.c | 100 ++--
 sequencer.h |   5 +-
 t/t3404-rebase-interactive.sh   |  32 +
 8 files changed, 186 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

[1] https://public-inbox.org/git/20170502040048.9065-1-liambeg...@gmail.com/
--
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aim to convert shell code to C.
Even if this is most likely not a big performance enhacement, let's
convert it too since a comming change to abbreviate command names requires
it to be updated.

Signed-off-by: Liam Beguin 
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 23 +--
 sequencer.c| 46 ++
 sequencer.h|  1 +
 4 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..9d94c874c5bb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+   ADD_EXEC
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+   OPT_CMDMODE(0, "add-exec", ,
+   N_("insert exec commands in todo list"), ADD_EXEC),
OPT_END()
};
 
@@ -64,5 +67,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!skip_unnecessary_picks();
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
+   if (command == ADD_EXEC && argc == 2)
+   return !!add_exec_commands(argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..760334d3a8b3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-   {
-   first=t
-   while read -r insn rest
-   do
-   case $insn in
-   pick)
-   test -n "$first" ||
-   printf "%s" "$cmd"
-   ;;
-   esac
-   printf "%s %s\n" "$insn" "$rest"
-   first=
-   done
-   printf "%s" "$cmd"
-   } <"$1" >"$1.new" &&
-   mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..810b7850748e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
return 0;
 }
 
+int add_exec_commands(const char *command)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   int fd, res, i, first = 1;
+   FILE *out;
+
+   strbuf_reset(_list.buf);
+   fd = open(todo_file, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s'"), todo_file);
+   if (strbuf_read(_list.buf, fd, 0) < 0) {
+   close(fd);
+   return error(_("could not read '%s'."), todo_file);
+   }
+   close(fd);
+
+   res = parse_insn_buffer(todo_list.buf.buf, _list);
+   if (res) {
+   todo_list_release(_list);
+   return error(_("unusable todo list: '%s'"), todo_file);
+   }
+
+   out = fopen(todo_file, "w");
+   if (!out) {
+   todo_list_release(_list);
+   return error(_("unable to open '%s' for writing"), todo_file);
+   }
+   for (i = 0; i < todo_list.nr; i++) {
+   struct todo_item *item = todo_list.items + i;
+   int bol = item->offset_in_buf;
+   const char *p = todo_list.buf.buf + bol;
+   int eol = i + 1 < todo_list.nr ?
+   todo_list.items[i + 1].offset_in_buf :
+   

[PATCH 4/5] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin 
Signed-off-by: Liam Beguin 
---
 Documentation/rebase-config.txt | 19 +++
 builtin/rebase--helper.c| 10 +---
 sequencer.c | 54 +
 sequencer.h |  4 +--
 4 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..0820b60f6e12 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,22 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase` will use abbreviated command names in the
+   todo list resulting in something like this:
+
+---
+   p deadbee The oneline of the commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
+   instead of:
+
+---
+   pick deadbee The oneline of the commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+   Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9d94c874c5bb..7b1fe825a877 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   int keep_empty = 0, abbreviate_commands = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +43,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
};
 
git_config(git_default_config, NULL);
+   git_config_get_bool("rebase.abbreviatecommands", _commands);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
@@ -56,11 +57,12 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+   return !!sequencer_make_script(keep_empty, abbreviate_commands,
+  stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
-   return !!transform_todo_ids(1);
+   return !!transform_todo_ids(1, abbreviate_commands);
if (command == EXPAND_SHA1S && argc == 1)
-   return !!transform_todo_ids(0);
+   return !!transform_todo_ids(0, abbreviate_commands);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 810b7850748e..aa01e8bd9280 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static const char command_to_char(const enum todo_command command)
+{
+   if (command < TODO_COMMENT && todo_command_info[command].c)
+   return todo_command_info[command].c;
+   return -1;
+}
+
 static int is_noop(const enum todo_command command)
 {
return TODO_NOOP <= command;
@@ -1242,15 +1249,16 @@ static int parse_insn_line(struct todo_item *item, 
const char *bol, char *eol)
return 0;
}
 
-   for (i = 0; i < TODO_COMMENT; i++)
+   for (i = 0; i < TODO_COMMENT; i++) {
if (skip_prefix(bol, todo_command_info[i].str, )) {
item->command = i;
break;
-   } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+   } else if (bol[1] == ' ' && *bol == command_to_char(i)) {
bol++;
item->command = i;
break;
}
+   }
if (i >= TODO_COMMENT)
return -1;
 
@@ -2443,8 +2451,8 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char 

[PATCH 5/5] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration options should not change anything else.

Signed-off-by: Liam Beguin 
---
 t/t3404-rebase-interactive.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..e460ebde3393 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'prepare rebase.abbreviateCommands' '
+   reset_rebase &&
+   git checkout -b abbrevcmd master &&
+   test_commit "first" file1.txt "first line" first &&
+   test_commit "second" file1.txt "another line" second &&
+   test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+   test_commit "squash! second" file1.txt "another line here" second_squash
+'
+
+cat >expected 

[PATCH 1/5] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin 
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 32 
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash entry
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash entry
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   This option can be overridden by the `--no-autostash` and
+   `--autostash` options of linkgit:git-rebase[1].
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command in the todo-list.
+   Defaults to "ignore".
+
+rebase.instructionFormat::
+   A format string, as specified in 

[PATCH 2/5] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin 
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
-   instruction list during an interactive rebase.  The format will
+   todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
-- 
2.15.0.321.g19bf2bb99cee.dirty

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Junio C Hamano]

Jeff King  writes:

>> cf. 
>> https://public-inbox.org/git/20171120202920.7ppcwmzkxifyw...@sigill.intra.peff.net/
>
> It's funny that we'd get two patches so close together. AFAIK the
> slowness here has been with us for years, and I just happened to
> investigate it recently.
>
>> The 5-patch series that contains the same change as this one is
>> cooking and will hopefully be in the released version before the end
>> of the year.
>
> I'd be curious if the 5th patch there provides an additional speedup for
> Takuto's case.

Indeed, it is a very good point.

IIUC, the 5th one is about fetching tons of refs that you have never
seen, right?  If a repository that has trouble with everything-local
is suffering because it right now has 300k remote-tracking branches,
I'd imagine that these remote-tracking branches are being added at a
considerable rate, so I'd not be surprised if these "new" refs
benefits from that patch.  And it would be nice to know how much a
real life scenario actually does improve.

Thanks.

Re: git status always modifies index?

[Jeff King]

On Sun, Nov 26, 2017 at 06:35:56PM +0900, Junio C Hamano wrote:

> Having a large picture option like "--read-only" instead of ending
> up with dozens of "we implemented a knob to tweak only this little
> piece, and here is an option to trigger it" would help users in the
> long run, but we traditionally did not do so because we tend to
> avoid shipping "incomplete" features, but being perfectionist with
> such a large undertaking can stall topics with feature bloat.  In a
> case like this, however, I suspect that an aspirational feature that
> starts small, promises little and can be extended over time may be a
> good way to move forward.

I actually consider "--no-optional-locks" to be such an aspirational
feature. I didn't go digging for other cases (though I'm fairly certain
that "diff" has one), but hoped to leave it for further bug reports ("I
used the option, ran command X, and saw lock contention").

I would be fine with having a further aspirational "read only" mode. As
I said before, that's not quite the same thing as no-optional-locks, but
I think they're close enough that I'd be fine having only one of them.
But now that we've shipped a version with the locking one, we're stuck
with at least for the duration of a deprecation cycle.

-Peff

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Jeff King]

On Mon, Nov 27, 2017 at 01:35:35PM +0900, Junio C Hamano wrote:

> Takuto Ikuta  writes:
> 
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 008b25d3db087..0184584e80599 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -716,7 +716,7 @@ static int everything_local(struct fetch_pack_args 
> > *args,
> > for (ref = *refs; ref; ref = ref->next) {
> > struct object *o;
> >  
> > -   if (!has_object_file(>old_oid))
> > +   if (!has_object_file_with_flags(>old_oid, 
> > OBJECT_INFO_QUICK))
> > continue;
> >  
> 
> It appears that great minds think alike?
> 
> cf. 
> https://public-inbox.org/git/20171120202920.7ppcwmzkxifyw...@sigill.intra.peff.net/

It's funny that we'd get two patches so close together. AFAIK the
slowness here has been with us for years, and I just happened to
investigate it recently.

> The 5-patch series that contains the same change as this one is
> cooking and will hopefully be in the released version before the end
> of the year.

I'd be curious if the 5th patch there provides an additional speedup for
Takuto's case.

-Peff

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Junio C Hamano]

Takuto Ikuta  writes:

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 008b25d3db087..0184584e80599 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -716,7 +716,7 @@ static int everything_local(struct fetch_pack_args *args,
>   for (ref = *refs; ref; ref = ref->next) {
>   struct object *o;
>  
> - if (!has_object_file(>old_oid))
> + if (!has_object_file_with_flags(>old_oid, 
> OBJECT_INFO_QUICK))
>   continue;
>  

It appears that great minds think alike?

cf. 
https://public-inbox.org/git/20171120202920.7ppcwmzkxifyw...@sigill.intra.peff.net/

The 5-patch series that contains the same change as this one is
cooking and will hopefully be in the released version before the end
of the year.

Re: [PATCH] Makefile: check that tcl/tk is installed

[Jeff King]

On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Perhaps the "else" part of the above should become a bit more
> > careful, something along the lines of...
> >
> > else
> > MSGFMT ?= msgfmt
> > -   ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; 
> > echo $$?),0)
> > -   MSGFMT := $(TCL_PATH) po/po2msg.sh
> > -   endif
> > +   MSGFMT_DRYRUN = --tcl -l C -d . /dev/null 2>/dev/null
> > +ifneq ($(shell $(MSGFMT) $(MSGFMT_DRYRUN); echo $$?),0)
> > +   ifneq ($(shell $(TCL_PATH) po/po2msg.sh $(MSGFMT_DRYRUN); 
> > echo $$?),0)
> > +MSGFMT := $(TCL_PATH) po/po2msg.sh
> > +   else
> > +   $(error "no usable msgfmt to build gitk; set NO_TCLTK 
> > perhaps?")
> > +   endif
> > endif
> > endif
> 
> Actually, at this point, I think the suggestion should primarily be
> to install either msgfmt or tclsh; offering another choice to set
> NO_TCLTK is OK, but it should be secondary, as the fact that the
> make utility is running this recipe is a sign that the builder wants
> to build gitk/git-gui.

I think that's the rub, though. We hit this code path by default, so
it's _not_ a sign that the builder cares about gitk.

I do agree that outlining "install one of these or disable tcl" as the
options is a good idea, though.

> Whether the builder wants to run the result on the same box is a
> separate and irrelevant matter.  Once built and installed, a box
> without "wish" may not be able to run the result, but installing it
> after the fact will fix it without need to rebuild anything.

Yeah, this side-steps the "other half" of the issue that Christian's
patch addresses, which seems like the more controversial part (I don't
have a strong opinion myself, though).

-Peff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Junio C Hamano]

Junio C Hamano  writes:

> Perhaps the "else" part of the above should become a bit more
> careful, something along the lines of...
>
> else
> MSGFMT ?= msgfmt
> -   ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; 
> echo $$?),0)
> -   MSGFMT := $(TCL_PATH) po/po2msg.sh
> -   endif
> +   MSGFMT_DRYRUN = --tcl -l C -d . /dev/null 2>/dev/null
> +ifneq ($(shell $(MSGFMT) $(MSGFMT_DRYRUN); echo $$?),0)
> +   ifneq ($(shell $(TCL_PATH) po/po2msg.sh $(MSGFMT_DRYRUN); 
> echo $$?),0)
> +MSGFMT := $(TCL_PATH) po/po2msg.sh
> +   else
> +   $(error "no usable msgfmt to build gitk; set NO_TCLTK 
> perhaps?")
> +   endif
> endif
> endif

Actually, at this point, I think the suggestion should primarily be
to install either msgfmt or tclsh; offering another choice to set
NO_TCLTK is OK, but it should be secondary, as the fact that the
make utility is running this recipe is a sign that the builder wants
to build gitk/git-gui.  

Whether the builder wants to run the result on the same box is a
separate and irrelevant matter.  Once built and installed, a box
without "wish" may not be able to run the result, but installing it
after the fact will fix it without need to rebuild anything.

[PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Takuto Ikuta]

Do not call prepare_packed_git for every refs.
In fetch-pack, git list ups entries in .git/objects/pack
directory for each refs.
Such behavior makes git fetch-pack slow for the repository having the
large number of refs, especially for windows.

For chromium repository, having more than 30 remote refs, git fetch
takes more than 3 minutes if fetch-pack runs on my windows workstation.
This patch improves the time to around 17 seconds in the same machine.

Signed-off-by: Takuto Ikuta 
---
 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 008b25d3db087..0184584e80599 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -716,7 +716,7 @@ static int everything_local(struct fetch_pack_args *args,
for (ref = *refs; ref; ref = ref->next) {
struct object *o;
 
-   if (!has_object_file(>old_oid))
+   if (!has_object_file_with_flags(>old_oid, 
OBJECT_INFO_QUICK))
continue;
 
o = parse_object(>old_oid);

--
https://github.com/git/git/pull/437

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

To recap (other than the typofix in the proposed log message), here
is what I would have as SQUASH??? on top of (or interspersed with)
v6.

Thanks.

diff --git a/http-backend.c b/http-backend.c
index 69570d16e7..2268d65731 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -324,10 +324,9 @@ static ssize_t read_request_fixed_len(int fd, ssize_t 
req_len, unsigned char **o
ssize_t cnt = 0;
 
if (max_request_buffer < req_len) {
-   die("request was larger than our maximum size (%lu): %lu;"
-   " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
-   max_request_buffer,
-   req_len);
+   die("request was larger than our maximum size (%lu): "
+   "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+   max_request_buffer, (uintmax_t)req_len);
}
 
if (req_len <= 0) {

diff --git a/Makefile b/Makefile
index e61f8319b3..3380f68040 100644
--- a/Makefile
+++ b/Makefile
@@ -661,7 +661,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
-TEST_PROGRAMS_NEED_X += test-print-values
+TEST_PROGRAMS_NEED_X += test-print-larger-than-ssize
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-write-cache

diff --git a/t/helper/test-print-values.c 
b/t/helper/test-print-larger-than-ssize.c
similarity index 31%
rename from t/helper/test-print-values.c
rename to t/helper/test-print-larger-than-ssize.c
index 8f7e5af319..b9852c493d 100644
--- a/t/helper/test-print-values.c
+++ b/t/helper/test-print-larger-than-ssize.c
@@ -1,10 +1,10 @@
-#include 
-#include 
+#include "cache.h"
 
 int cmd_main(int argc, const char **argv)
 {
-   if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
-   printf("%zu", (ssize_t)(-20));
+   size_t large = ~0;
 
+   large = ~(large & ~(large >> 1)) + 1;
+   printf("%" PRIuMAX "\n", (uintmax_t) large);
return 0;
 }

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index f452090216..112b5d6eb2 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -72,7 +72,7 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
 '
 
 # overrides existing definition for further cases
-run_backend() {
+run_backend () {
CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
( echo "$2" && cat /dev/zero ) |
QUERY_STRING="${1#*[?]}" \
@@ -89,7 +89,7 @@ test_expect_success 'CONTENT_LENGTH set and infinite input' '
 '
 
 test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
-   NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" 
"(size_t)(-20)"` &&
+   
NOT_FIT_IN_SSIZE=$("$GIT_BUILD_DIR/t/helper/test-print-larger-than-ssize") &&
env \
CONTENT_TYPE=application/x-git-upload-pack-request \
QUERY_STRING=/repo.git/git-upload-pack \

Re: [PATCH] merge-recursive: ignore_case shouldn't reject intentional removals

[Junio C Hamano]

Elijah Newren  writes:

> In commit ae352c7f3 (merge-recursive.c: fix case-changing merge bug,
> 2014-05-01), it was observed that removing files could be problematic on
> case insensitive file systems, because we could end up removing files
> that differed in case only rather than deleting the intended file --
> something that happened when files were renamed on one branch in a way
> that differed only in case.  To avoid that problem, that commit added
> logic to avoid removing files other than the one intended, rejecting the
> removal if the files differed only in case.
>
> Unfortunately, the logic it used didn't fully implement that condition as
> stated above...
>
> If that description leaves more questions than answers, we may need to
> augment the above commit message with the following explanation...
> ...
>  merge-recursive.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

As a fix, this sorely wants something new in t/ directory.

Re: [PATCH 2/2] trace: improve performance while category is disabled

[Junio C Hamano]

Junio C Hamano  writes:

> Just in case others notice style and whitespace issues, I've applied
> the following to fix them, so there is no need to reroll only to fix
> these.
> ...
> -inline int trace_pass_fl(struct trace_key *key) {
> +inline int trace_pass_fl(struct trace_key *key)
> +{
>   return key->fd || !key->initialized;
>  }

Spotted yet another.  This function in a header file, that is
included by many source files, must be made "static inline" (which I
already did as without the fix I couldn't get 'pu' to compile).

Re: [PATCH] grep: Add option --max-line-len

[Junio C Hamano]

Marc-Antoine Ruel  writes:

> [second try, now with text format]
>
> Thanks a lot for the reviews. Replying to both.
>
> If I send a follow up, I'll fix the commit description and the help
> string, remove the shorthand -M, write a more sensible test.
>
> ...
>
> Thanks for the thoughtful analysis. My main motivation was (1), thus
> filtering with a pathspec is much better than trying to work around
> the issue. The issues raised in the review are significant enough that
> committing this patch could cause significant issues; I don't know how
> to resolve handling with -v and how to handle tabs.

OK.  Thanks for thinking it through.

Re: [PATCH] grep: Add option --max-line-len

[Marc-Antoine Ruel]

[second try, now with text format]

Thanks a lot for the reviews. Replying to both.

If I send a follow up, I'll fix the commit description and the help
string, remove the shorthand -M, write a more sensible test.


2017-11-23 14:24 GMT-05:00 Eric Sunshine :
>> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
>> @@ -127,6 +128,10 @@ OPTIONS
>> +-M::
>> +--max-line-len::
>> +   Match the pattern only for line shorter or equal to this length.
>
> This documentation doesn't explain what it means by a line's length.
> This implementation seems to take into consideration only the line's
> byte count, however, given that displayed lines are normally
> tab-expanded, people might intuitively expect that expansion to count
> toward the length. A similar question arises for Unicode characters.
>
> Should this option take tab-expansion and Unicode into account?
> Regardless of the answer to that question, the documentation should
> make it clear what "line length" means.

Excellent question. I don't have an immediate answer.


>> diff --git a/grep.c b/grep.c
>> @@ -1273,6 +1275,8 @@ static int match_line(struct grep_opt *opt, char *bol, 
>> char *eol,
>> +   if (opt->max_line_length > 0 && eol-bol > opt->max_line_length)
>> +   return 0;
>
> If the user specifies "-M0", should that error out or at least warn
> the user that the value is non-sensical? What about -1, etc.? (These
> are UX-related questions; the implementation obviously doesn't care
> one way or the other.)

Precedent with -A is to ignore the negative value. I don't have a
strong opinion.


2017-11-23 20:44 GMT-05:00 Junio C Hamano :
>
> Marc-Antoine Ruel  writes:
>
> > This tells git grep to skip files longer than a specified length,
> > which is often the result of generators and not actual source files.
> >
> > ...
> > +-M::
> > +--max-line-len::
> > + Match the pattern only for line shorter or equal to this length.
> > +
>
> All the excellent review comments from Eric I agree with.
>
> With the name of the option and the above end-user facing
> description, it is very clear that the only thing this feature does
> is to declare that an overlong line does _not_ match when trying to
> check against any pattern.
>
> That is a much clearer definition and description than random new
> features people propose here (and kicked back by reviewers, telling
> them to make the specification clearer), and I'd commend you for that.
>
> But it still leaves at least one thing unclear.  How should it
> interact with "-v"?  If we consider an overlong line never matches,
> would "git grep -v " should include the line in its output?

Ah! No idea. :/

> Speaking of the output, it also makes me wonder if the feature
> really wants to include an overlong line as a context line when
> showing a near-by line that matches the pattern when -A/-B/-C/-W
> options are in use. Even though it is clear that it does from the
> above description, is it really the best thing the feature can do to
> help the end users?
>
> Which leads me to suspect that this "feature" might not be the ideal
> you wanted to achive, but is an approximate substitution that you
> found is "good enough" to simulate what the real thing you wanted to
> do, especially when I go back and read the justfication in the
> proposed log message that talks about "result of generators".
>
> Isn't it a property of the entire file, not individual lines, if you
> find it uninteresting to see reported by "git grep"?  I cannot shake
> the suspicion that this feature happened to have ended up in this
> shape, instead of "ignore a file with a line this long", only
> because your starting point was to use "has overlong lines" as the
> heuristic for "not interesting", and because "git grep" code is not
> structured to first scan the entire file to decide if it is worth
> working on it, and it is extra work to restructure the codeflow to
> make it so (which you avoided).
>
> If your real motivation was either
>
>  (1) whether the file has or does not have the pattern for certain
>  class of files are uninteresting; do not even run "grep"
>  processing for them; or
>
>  (2) hits or no-hits may be intereseting but output of overlong
>  lines from certain class of files I do not wish to see;
>
> then I can think of two alternatives.
>
> For (1), can't we tell "result of generators" and other files with
> pathspec?  If so, perhaps a negative pathspec can rescue.  e.g.
>
> git grep  -- '*.cc' ':!*-autogen.cc'
>
> For (2), can't we model this after how users can tell "git diff"
> that certain paths are not worth computing and showing textual
> patches for, which is to Unset the 'diff' attribute?  When you have
>
> *-autogen.cc-diff
>
> in your .gitattributes, "git diff" would say "Binary files A and B
> differ" instead of explaining line-by-line differences in the patch
> form.  Perhaps we can also 

Re: Problem with environment of hook execution when git is run with --work-tree / --git-dir

[Michael Sloan]

On Sun, Nov 26, 2017 at 5:16 PM, Junio C Hamano  wrote:
> Michael Sloan  writes:
>
>> So what's the problem with this choice of environment variables?
>> Well, the problem is that if PWD is changed during the execution of
>> the script, then GIT_WORK_TREE and GIT_DIR will no longer work
>> properly. For example, if the pre-commit hook is
>>
>> #!/bin/sh
>> cd some-dir
>> git status
>>
>> This will fail with
>>
>> Not a git repository: '.dotfiles-git'
>
> That is to be expected.  It's up to the script to make them absolute
> if it cannot cope with relative paths.
>
>> There is another detail here, which is that when --git-dir /
>> --work-tree is not specified, the no GIT_WORK_TREE / GIT_DIR
>> environment variable is set.  This means that in this case, changing
>> PWD in the hook will work fine as long as the search for .git will
>> find the right one.
>
> That also is working as designed.

Hmm, I do not think that this is good for the reliability of hooks.
It means that hooks written with the common case assumption (no
GIT_WORK_TREE / GIT_DIR set) will fail when run in these rarer
configurations.  I imagine that many authors of hooks are entirely
unaware of work-tree / git-dir options.  I know that I used git for
something like 8 years before encountering a use for them, or really
being aware.  Perhaps hooks authors are savvier than your average
user.

It seems to me like something similar to my suggested GIT_DIR_MAPPINGS
could be quite powerful for this circumstance as well as others.  I
guess a temporary hack would be to create a ".git" file that specifies
the git-dir, but this doesn't work if something is already called
".git".

I do not have a concrete example of this causing problems in practice,
I've just observed that it is a potential gotcha for --git-dir +
hooks.  I can understand keeping things simple even if it means making
it much harder for hooks authors to write correct code.  It seems
bothersome to me that git hooks have to be crafted so carefully to
support variations in environment.

If we could go back to when hooks were introduced and add a
"GIT_IN_HOOK=1" and have it require manual specification of
--work-tree and -git-dir, that might have been the best option.
However, doing that now would break everyone's hooks, so not really
practical.

Also, thanks for all your work on git, overall it is excellent software :)

-Michael

Re: [PATCH] pretty: fix buffer over-read with %> and %

[Junio C Hamano]

mwnx  writes:

> diff --git a/pretty.c b/pretty.c
> index 2f6b0ae6c..4c70bad45 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf 
> *sb,
>   const char *end = start + strcspn(start, ",)");
>   char *next;
>   int width;
> - if (!end || end == start)
> + if (!*end || end == start)

Yuck.  This is so obvious a typo as it is quite clear that a few
lines above will never give us !end.  Well spotted.

By the way, Documentation/SubmittingPatches has this in "(5) Certify
your work" section:

Also notice that a real name is used in the Signed-off-by: line. Please
don't hide your real name.


>   return 0;
>   width = strtol(start, , 10);
>   if (next == start || width == 0)
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 591f35daa..4d9555962 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'unterminated alignment formatting' '
> + git log -n1 --format="%<(42" >actual &&
> + echo "%<(42" >expected &&
> + test_cmp expected actual
> +'
> +
>  test_done

Re: [PATCH 2/2] trace: improve performance while category is disabled

[Junio C Hamano]

Junio C Hamano  writes:

> gennady.kup...@gmail.com writes:
>
>> From: Gennady Kupava 
>>
>> - Do the check if the trace key is enabled sooner in call chain.
>> - Move just enough code from trace.c into trace.h header so all code
>>   necessary to determine that trace is disabled could be inlined to
>>   calling functions.
>
> Makes sense.  Will queue.
>
> Thanks.

Just in case others notice style and whitespace issues, I've applied
the following to fix them, so there is no need to reroll only to fix
these.

diff --git a/trace.h b/trace.h
index db10f2afeb..05395242aa 100644
--- a/trace.h
+++ b/trace.h
@@ -85,29 +85,29 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
if (trace_pass_fl(key)) \
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,   \
__VA_ARGS__);   \
-   } while(0)
+   } while (0)
 
 #define trace_printf(...) trace_printf_key(_default_key, __VA_ARGS__)
 
 #define trace_argv_printf(argv, ...)   \
do {\
if (trace_pass_fl(_default_key))  \
-  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\
+   trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,   \
argv, __VA_ARGS__); \
-   } while(0)
+   } while (0)
 
 #define trace_strbuf(key, data)
\
do {\
if (trace_pass_fl(key)) \
trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
-   } while(0)
+   } while (0)
 
 #define trace_performance(nanos, ...)  \
do {\
if (trace_pass_fl(_perf_key)) \
trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\
-__VA_ARGS__);  \
-   } while(0)
+__VA_ARGS__);  \
+   } while (0)
 
 #define trace_performance_since(start, ...)\
do {\
@@ -115,7 +115,7 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
trace_performance_fl(TRACE_CONTEXT, __LINE__,   \
 getnanotime() - (start),   \
 __VA_ARGS__);  \
-   } while(0)
+   } while (0)
 
 /* backend functions, use non-*fl macros instead */
 __attribute__((format (printf, 4, 5)))
@@ -129,7 +129,8 @@ extern void trace_strbuf_fl(const char *file, int line, 
struct trace_key *key,
 __attribute__((format (printf, 4, 5)))
 extern void trace_performance_fl(const char *file, int line,
 uint64_t nanos, const char *fmt, ...);
-inline int trace_pass_fl(struct trace_key *key) {
+inline int trace_pass_fl(struct trace_key *key)
+{
return key->fd || !key->initialized;
 }
 

Re: [PATCH 2/2] trace: improve performance while category is disabled

[Junio C Hamano]

gennady.kup...@gmail.com writes:

> From: Gennady Kupava 
>
> - Do the check if the trace key is enabled sooner in call chain.
> - Move just enough code from trace.c into trace.h header so all code
>   necessary to determine that trace is disabled could be inlined to
>   calling functions.

Makes sense.  Will queue.

Thanks.

> +inline int trace_pass_fl(struct trace_key *key) {
> + return key->fd || !key->initialized;
> +}

;-)

Re: Problem with environment of hook execution when git is run with --work-tree / --git-dir

[Junio C Hamano]

Michael Sloan  writes:

> So what's the problem with this choice of environment variables?
> Well, the problem is that if PWD is changed during the execution of
> the script, then GIT_WORK_TREE and GIT_DIR will no longer work
> properly. For example, if the pre-commit hook is
>
> #!/bin/sh
> cd some-dir
> git status
>
> This will fail with
>
> Not a git repository: '.dotfiles-git'

That is to be expected.  It's up to the script to make them absolute
if it cannot cope with relative paths.

> There is another detail here, which is that when --git-dir /
> --work-tree is not specified, the no GIT_WORK_TREE / GIT_DIR
> environment variable is set.  This means that in this case, changing
> PWD in the hook will work fine as long as the search for .git will
> find the right one.

That also is working as designed.

Re: [PATCH] Makefile: check that tcl/tk is installed

[Junio C Hamano]

Jeff King  writes:

> ...
> I think your patch does say "consider setting NO_TCLTK" in that case,
> which is an improvement. But it might be nicer still if it Just Worked
> (either because we don't do tcl/tk by default, or because we respect
> NO_GETTEXT in the gitk/git-gui Makefiles, or because our msgfmt can
> fallback further to not even using tclsh).
>
> So I'm not really against this patch, but IMHO it doesn't make the
> interesting case (you don't care about tcl and are just trying to build
> git for the first time) all that much better.

I was in agreement with that line of argument while the patch was
discussed, and I still think the above is true.

And if the primary thing we want to address with this patch is the
build breakage for those who do not have tclsh nor msgfmt, perhaps
it is a more direct fix to update this part in gitk/Makefile:

## po-file creation rules
XGETTEXT   ?= xgettext
ifdef NO_MSGFMT
MSGFMT ?= $(TCL_PATH) po/po2msg.sh
else
MSGFMT ?= msgfmt
ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; 
echo $$?),0)
MSGFMT := $(TCL_PATH) po/po2msg.sh
endif
endif

There is an identical copy-and-pasted copy of this in
git-gui/Makefile, and both of them silently assume that TCL_PATH
points at something usable when msgfmt is not available.

Perhaps the "else" part of the above should become a bit more
careful, something along the lines of...

else
MSGFMT ?= msgfmt
-   ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; 
echo $$?),0)
-   MSGFMT := $(TCL_PATH) po/po2msg.sh
-   endif
+   MSGFMT_DRYRUN = --tcl -l C -d . /dev/null 2>/dev/null
+ifneq ($(shell $(MSGFMT) $(MSGFMT_DRYRUN); echo $$?),0)
+   ifneq ($(shell $(TCL_PATH) po/po2msg.sh $(MSGFMT_DRYRUN); echo 
$$?),0)
+MSGFMT := $(TCL_PATH) po/po2msg.sh
+   else
+   $(error "no usable msgfmt to build gitk; set NO_TCLTK 
perhaps?")
+   endif
endif
endif


Another reason why I think the Makefiles in gitk and git-gui
projects are better targets for the fix is because they are designed
to be independent projects.  We should be able to do

cd git-gui && make

in our tree, but an update to our top-level Makefile does not help
those people.

Re: git status always modifies index?

[Junio C Hamano]

Jeff King  writes:

> I'm not sure I agree. Lockless writes are actually fine for the original
> use case of --no-optional-locks (which is a process for the same user
> that just happens to run in the background).

The phrase "lockless write" scares me---it sounds as if you
overwrite the index file no matter what other people (including
another instance of yourself) are doing to it.  

Side note: What 'use-optional-locks' actually does is not to
give any file descriptor to write into when we invoke the
wt-status helpers (which would want to make an opportunistic
update to the index under the lock), so "--no-optional-locks" is
quite different from "lockless write".  Whew.  It is part of
what "semantically read-only things do not write" would have
been.

How would a true "lockless write" that is OK for background
opportunistic refresh work?  Read, compute and then open the final
index file under its final name for writing and write it out,
without involving any rename?  As long as it finishes writing the
result in full and closes, its competing with a real "lockful write"
would probably be safe when it loses (the lockful one will rename
its result over to the refreshed one).  It cannot "win" the race by
writing into the temporary lock file the other party is using ;-)
But it may lose the race in a messy way---the lockful one tries to
rename its result over to the real index, which the lockless one has
still open and writing.  Unix variants are probably OK with it and
the lockless one would lose gracefully, but on other platforms the
lockful one would fail to rename, I suspect?  Or the lockless one
can crash while it is writing even if there is no race.

Or do you mean something different by "lockless write"?

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Junio C Hamano]

Max Kirillov  writes:

> Add tests for cases:
>
> * CONTENT_LENGTH is set, script's stdin has more data.
>   (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
>   and fail. It does not seem to cause any performance issues with the default
>   value of GIT_HTTP_MAX_REQUEST_BUFFER.)
> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

s/fix/fit/ you meant?

> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
> new file mode 100644
> index 00..8f7e5af319
> --- /dev/null
> +++ b/t/helper/test-print-values.c
> @@ -0,0 +1,10 @@
> +#include 
> +#include 
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
> + printf("%zu", (ssize_t)(-20));
> +
> + return 0;
> +}

As far as I know, we avoid %zu (C99), as it may not be safe yet to
do so on all platforms.

e.g. c.f. 
https://public-inbox.org/git/64c7d52f-9030-460c-8f61-4076f5c1d...@gmail.com/

You may want to double check the 1/2 of this topic, too.

Forcing a test command line to spell out "(size_t)(-20)" feels a bit
atrocious, especially given that this program is capable of ever
showing that string and nothing else, and it does not even diagnose
typos as errors.

I wonder if we would want to have "test-print-larger-than-ssize" and
do something like

#include "cache.h"

int cmd_main(int ac, const char **av)
{
uintmax_t large = ((uintmax_t) SSIZE_MAX) + 1;

printf("%" PRIuMAX "\n", large);
return 0;
}

perhaps?

Note that wrapper.c seems to assume that not everybody has
SSIZE_MAX, so we might have to do something silly like

size_t large = ~0;
large = ~(large & ~(large >> 1)) + 1;

printf("%" PRIuMAX "\n", (uintmax_t) large);

just to be careful (even though we now assume 2's complement),
though.

[SCRIPT/RFC 1/3] setup.sh

[Igor Djordjevic]

On 26/11/2017 23:35, Igor Djordjevic wrote:
> 
> This is what we end up with once "master" and topic branches are 
> merged in merge commit M1 inside temporary "test" branch for further 
> integration testing:
> 
> (2)o---o---A (topicA)
>   / \
>  /   M1 (test, HEAD)
> /   /||
> ---o---o---M---/ || (master)
> \   \   / |
>  \   o---B-/  | (topicB)
>   \   |
>o---o---C--/ (topicC)

To begin with, you can use provided "setup.sh"[*1*] script, putting 
you straight into position shown on graph (2) above, with addition of 
tag "A" and remote branch "origin/topicA" so you could try using 
these as "--onto-parent" values, too.

As seen in there, change "X" is already made and staged, so you can 
now just run something like:

git-commit--onto-parent.sh --onto-parent topicA

... to see the logic in action.
 
Instead of "topicA", you may try providing tag "A", remote branch 
"origin/topicA" or even plain commit hash. It`s interesting to add 
"--amend" option into the mix, too, and see what happens. Also, you 
can try using "topicB" and see the commit fail (as it doesn`t merge 
cleanly).

All this while "test.txt" file doesn`t get modified on disk (nor 
would any other file) - being desired behaviour, as we didn`t 
actually change anything inside the working tree, but just amended 
history of how we got here, so recompilation isn`t needlessly 
triggered :)

p.s. Note these two lines near the end:

sed -i '4iX1' test.txt  # works with simple patch apply
sed -i '17iX2' test.txt # needs three-way file merge

You can play with it, commenting out one or the other and observing 
how it influences "git commit --onto-parent" in regards of the parent 
provided.

Regards, Buga

[*1*] "setup.sh", can clean previous setup run as well, but commented 
 out here for safety, not to unexpectedly delete something for unwary 
 user.
--- 8< ---
#!/bin/sh

#rm -rf ./.git
#rm -f ./test.txt

git init

touch ./test.txt
git add -- test.txt

for i in {1..10}
do
echo $i >>test.txt  
git commit -am "$i"
done

echo M >>test.txt
git commit -am "M"

git checkout -b topicA HEAD~2

for i in 1 2
do
sed -i "${i}iA${i}" test.txt
git commit -am "A$i"
done
sed -i '3iA' test.txt
git commit -am "A"
git tag A

# simulate remote branch
mkdir -p ./.git/refs/remotes/origin &&
echo $(git rev-parse HEAD^0) >$_/topicA

git checkout -b topicB master^

sed -i '4iB1' test.txt
git commit -am "B1"
sed -i '5iB' test.txt
git commit -am "B"

git checkout -b topicC master~2

for i in 1 2
do
j=`expr "$i" + 5`
sed -i "${j}iC${i}" test.txt
git commit -am "C$i"
done
sed -i "8iC" test.txt
git commit -am "C"

git checkout -b test master
git merge --no-edit topicA topicB topicC

sed -i '4iX1' test.txt  # works with simple patch apply
sed -i '17iX2' test.txt # needs three-way file merge
git add -- test.txt

echo
git log --all --decorate --oneline --graph
echo
git diff --cached

[SCRIPT/RFC 2/3] git-merge-one-file--cached

[Igor Djordjevic]

Original "git-merge-one-file" script is slightly tweaked here into 
"git-merge-one-file--cached"[*1*] to allow (still trivial) _index-only_ 
three-way file merge, not touching files inside working tree.

Proof of concept, not thoroughly tested, original content left in, 
commented out. Feel free to comment/polish.

To make it available, I guess it would be best placed beside existing 
"git-merge-one-file" script...?

Regards, Buga

[*1*] "git-merge-one-file--cached"
--- 8< ---
#!/bin/sh
#
# Copyright (c) Linus Torvalds, 2005
#
# ---
# Original "git-merge-one-file" script slightly tweaked into
# "git-merge-one-file--cached" to allow (still trivial) index-only
# three-way file merge, not touching files inside working tree.
#
# Proof of concept, not thoroughly tested, original content left in,
# commented out.
# ---
#
# This is the git per-file merge script, called with
#
#   $1 - original file SHA1 (or empty)
#   $2 - file in branch1 SHA1 (or empty)
#   $3 - file in branch2 SHA1 (or empty)
#   $4 - pathname in repository
#   $5 - original file mode (or empty)
#   $6 - file in branch1 mode (or empty)
#   $7 - file in branch2 mode (or empty)
#
# Handle some trivial cases.. The _really_ trivial cases have
# been handled already by git read-tree, but that one doesn't
# do any merges that might change the tree layout.

USAGE='   '
USAGE="$USAGE   "
LONG_USAGE="usage: git merge-one-file $USAGE

Blob ids and modes should be empty for missing files."

SUBDIRECTORY_OK=Yes
. git-sh-setup
cd_to_toplevel
require_work_tree

if test $# != 7
then
echo "$LONG_USAGE"
exit 1
fi

case "${1:-.}${2:-.}${3:-.}" in
#
# Deleted in both or deleted in one and unchanged in the other
#
"$1.." | "$1.$1" | "$1$1.")
if { test -z "$6" && test "$5" != "$7"; } ||
   { test -z "$7" && test "$5" != "$6"; }
then
echo "ERROR: File $4 deleted on one branch but had its" >&2
echo "ERROR: permissions changed on the other." >&2
exit 1
fi

if test -n "$2"
then
echo "Removing $4"
# else
# read-tree checked that index matches HEAD already,
# so we know we do not have this path tracked.
# there may be an unrelated working tree file here,
# which we should just leave unmolested.  Make sure
# we do not have it in the index, though.
# exec git update-index --remove -- "$4"
fi
# if test -f "$4"
# then
# rm -f -- "$4" &&
# rmdir -p "$(expr "z$4" : 'z\(.*\)/')" 2>/dev/null || :
# fi &&
exec git update-index --remove --cacheinfo "$6","$2","$4"
;;

#
# Added in one.
#
".$2.")
# the other side did not add and we added so there is nothing
# to be done, except making the path merged.
exec git update-index --add --cacheinfo "$6","$2","$4"
;;
"..$3")
echo "Adding $4"
if test -f "$4"
then
echo "ERROR: untracked $4 is overwritten by the merge." >&2
exit 1
fi
git update-index --add --cacheinfo "$7","$3","$4" # &&
# exec git checkout-index -u -f -- "$4"
;;

#
# Added in both, identically (check for same permissions).
#
".$3$2")
if test "$6" != "$7"
then
echo "ERROR: File $4 added identically in both branches," >&2
echo "ERROR: but permissions conflict $6->$7." >&2
exit 1
fi
echo "Adding $4"
git update-index --add --cacheinfo "$6","$2","$4" # &&
# exec git checkout-index -u -f -- "$4"
;;

#
# Modified in both, but differently.
#
"$1$2$3" | ".$2$3")

case ",$6,$7," in
*,12,*)
echo "ERROR: $4: Not merging symbolic link changes." >&2
exit 1
;;
*,16,*)
echo "ERROR: $4: Not merging conflicting submodule changes." >&2
exit 1
;;
esac

src1=$(git unpack-file $2)
src2=$(git unpack-file $3)
case "$1" in
'')
echo "Added $4 in both, but differently."
orig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
;;
*)
echo "Auto-merging $4"
orig=$(git unpack-file $1)
;;
esac

git merge-file "$src1" "$orig" "$src2"
ret=$?
msg=
if test $ret != 0 || test -z "$1"
then
msg='content conflict'
ret=1
fi

# Create the working tree file, using "our tree" version from the
# index, and then store the result of the merge.
# git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
# rm -f -- "$orig" "$src1" "$src2"

if test "$6" != "$7"
   

[SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Igor Djordjevic]

Hi to all,

Here`s my humble attempt on (once more) scratching a use case which 
seems to be desired occasionally through the years, obviously not 
enough to be actually implemented yet, but might be worth some more 
attention... :)

For the reference, some past mentions of (more or less) similar ideas 
are listed at the end of this e-mail[*1*], where I`m also Cc-ing 
people previously involved, might be still sharing interest - as this 
is my first "new thread" list message, I`m sorry if this is not an 
encouraged practice, please let me know.

Approach discussed here could have a few more useful applications, 
but one seems to be standing out the most - in case where multiple 
topic branches are temporarily merged for integration testing, it 
could be very useful to be able to post "hotfix" commits to merged 
branches directly, _without actually switching to them_ (and thus 
without touching working tree files), and still keeping everything 
merged, in one go.

Example starting point is "master" branch with 3 topic branches (A, 
B, C), to be (throwaway) merged for integration testing inside 
temporary "test" branch:

(1)o---o---A (topicA)
  /
 /
/
---o---o---M (master, test, HEAD)
\   \
 \   o---B (topicB)
  \
   o---o---C (topicC)


This is what we end up with once "master" and topic branches are 
merged in merge commit M1 inside temporary "test" branch for further 
integration testing:

(2)o---o---A (topicA)
  / \
 /   M1 (test, HEAD)
/   /||
---o---o---M---/ || (master)
\   \   / |
 \   o---B-/  | (topicB)
  \   |
   o---o---C--/ (topicC)


Upon discovery of a fix needed inside "topicA", hotfix changes X 
should be committed to "topicA" branch and re-merged inside merge 
commit M2 on temporary integration "test" branch (previous temporary 
merge commit M1 is thrown away as uninteresting):

(3)o---o---A---X (topicA)
  / \
 /   M2 (test, HEAD)
/   /||
---o---o---M---/ || (master)
\   \   / |
 \   o---B-/  | (topicB)
  \  /
   o---o---C/ (topicC)


Now, usually advised approach to get from (2) to (3), where one is 
expected to switch to desired topic branch, commit the change, and 
then re-merge everything again into integration testing branch, is 
arguably a bit tiresome, but where it actually fails short is the 
fact that branch switching back and forth (for each hotfix commit) 
could possibly keep changing a lot of files otherwise untouched by 
the hotfix changes we`re committing (but different between branches), 
causing build systems to needlessly waste what could be significant 
time compiling them again and again.

Example script proposes using something like this instead:

(4) git commit --onto-parent topicA
 
... in above-mentioned case (2) to commit X onto "topicA" branch 
directly, re-merging all previously merged integration testing topic 
branches at the same time, reaching state (3) without any 
intermediate branch switching (and without touching working tree, 
thus without needless recompilation).

Once integration tests pass, integration test branch will be thrown 
away and new commits on each topic branch should still be properly 
tested - we`re just deferring it not to do it in the middle of 
integration testing, saving some (or a lot) needless rebuild cycles 
at the same time (or in the first place, even).

Scripts in series:
  [1/3]: setup.sh
  [2/3]: git-merge-one-file--cached
  [3/3]: git-commit--onto-parent.sh

Regards, Buga

[*1*] Some previous list mentions of similar functionality, in order 
 of appearance, latest on top (I kind of remember seeing more, but 
 can`t find them now, please feel free to add here, or notify more 
 people interested in the past):

 - [PATCH/RFC] git-post: the opposite of git-cherry-pick (2017-10-05)
   https://public-inbox.org/git/c6b52120-98bf-d685-6dc0-3c83e9e80...@kdbg.org/

 - "groups of files" in Git? (2017-07-11)
   
https://public-inbox.org/git/caeceraz3vyekvj8sm1ffdavsp3lmvqa1o3yojvthvg-0fpt...@mail.gmail.com/

 - Making a (quick) commit to another branch (2013-04-27)
   https://public-inbox.org/git/517bdb6d.8040...@cedarsoft.com/

 - Commit to other branch (2010-05-31)
   https://public-inbox.org/git/4c03d9c1.1060...@cedarsoft.com/

 - [RFC] git commit --branch (2006-05-29)
   https://public-inbox.org/git/20060529202851.ge14...@admingilde.org/

 - n-heads and patch dependency chains (2006-04-03)
   https://public-inbox.org/git/4430d352.4010...@vilain.net/

 - Multi-headed branches (hydra? :)) for basic patch calculus (2006-04-02)
   https://public-inbox.org/git/1143950852.21233.23.camel@localhost.localdomain/

[SCRIPT/RFC 3/3] git-commit--onto-parent.sh

[Igor Djordjevic]

Finally, "git-commit--onto-parent.sh"[*1*] shows an initial script 
version for you to examine, test out and hopefully comment on :)

Especially interesting part might be index-only three-way file merge, 
through usage of "git-merge-one-file--cached" script. Of course, this 
still only works for some trivial resolutions, where in case of more 
complex ones, involving unresolved conflicts, we back-out and fail. 
Still, it should be more powerful than `git-apply`.

Consider this proof of concept and work in progress, an idea where 
I`d like feedback on everything you come up with or find interesting, 
even parameter name possibly used instead of "--onto-parent" (and its 
short version), or approach in general.

For example, it might make sense to separate commit creation (on 
current HEAD`s parent) and its actual re-merging into integration 
test branch, where "--remerge" (or something) parameter would be used 
on top of "--onto-parent" to trigger both, if/when desired.

Another direction to think in might be introducing more general 
"--onto" parameter, too (or instead), without "parent" restriction, 
allowing to record a commit on top of any arbitrary commit (other 
than HEAD). This could even be defaulted to "git commit " 
(no option needed), where current "git commit" behaviour would then 
just be a special case of omitted  defaulting to HEAD, 
aligning well with other Git commands sharing the same behaviour.

Alas, rewind to present...

Please do note that I`m still relatively new to Git, and pretty new 
to both Linux and scripting in general (on Windows as well), and the 
whole concept of open-source software contributing, even, so please 
bare with me (or at least don`t get upset too much, lol), and do feel 
free to share your thoughts and remarks, even the trivial or harsh 
ones -- I`m grateful to learn and expand my knowledge, hopefully 
producing something useful in return :) Heck, might be I`m totally 
off-track here as well.

p.s. For some context - nowadays I mostly work in Delphi, and 
occasionally in C#, though through last 20 years I`ve been involved 
with C, Pascal, Basic, but also PHP, JavaScript, and whatnot - even 
good old assembly from time to time, when needed :)

Regards, Buga

[*1*] "git-commit--onto-parent.sh", probably too heavily commented in 
 the first place, but as I`m new to everything here I kind of feel the 
 plain words might unfortunately describe my intention a bit better 
 than my code, for now at least.
--- 8< ---
#!/bin/sh
#
# Copyright (c) 2017 Igor Djordjevic

i=$#
while test $i != 0
do
#
# Parameter parsing might be uninteresting here, as the whole
# script is currently just a wrapper around `git commit`, for a
# functionality that conceptually belongs there directly.
#
case "$1" in
--onto-parent=*)
onto_parent="${1#*=}"

shift && i=$(expr $i - 1)
;;
--onto-parent)
shift && i=$(expr $i - 1)
onto_parent="$1"

shift && i=$(expr $i - 1)
;;
-a|--a|--al|--all)
all=t

#
# For now, `git commit` "--all" option is special-cased in
# terms of being stripped out of the original command line
# (to be passed to `git commit`) and processed manually, as
# once commit is to be made, due to states of index and
# working tree, "--all" is most probably NOT what the user
# wants nor expects ;)
#
shift && i=$(expr $i - 1)
;;
*)
# parameters to pass down to `git commit`
set -- "$@" "$1"
shift && i=$(expr $i - 1)
;;
esac
done

main () {
#
# Store current HEAD (ref or commit) and verify that
# --onto-parent is valid and amongst its parents.
#
head="$(git symbolic-ref --short --quiet HEAD)" ||
head="$(git rev-parse HEAD^0)" &&
verify_onto_parent "$head" "$onto_parent" || exit 1

#
# As both HEAD and "--onto-parent" could be refs, where underlying
# commits could change, store original commits for later parents
# processing, getting updated parent list for new/updated
# merge commit.
#
head_commit="$(git rev-parse "$head"^0)" &&
onto_parent_commit="$(git rev-parse "$onto_parent"^0)" || exit 1

#
# Custom processing of stripped "--all" parameter - if we were to
# just pass it to `git commit`, "--all" would most probably yield
# an unexpected result in the eyes of the user, as it would include
# _all changes from all the other merge commit parents as well_,
# not just the changes we may actually wanted to "push down"
# (commit) onto specified parent (what would `git diff` show),
# 

Ich werde auf Ihre Antwort warten

[Mr.Yuehan Pan]

Schönen Tag,

Ich bin Herr Yuehan Pan, Direktor der Bank of China
Ich suche einen Manager / Investitionspartner, der mit mir für ein
gegenseitiges Geschäftsunternehmen arbeiten wird.

Bitte kontaktieren Sie mich in meiner privaten E-Mail für weitere Details.
E-Mail (yuehanpa...@gmail.com)

Ich warte darauf, etwas von dir zu hören.

Vielen Dank,

Herr Yuhan Pan`

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

On Sun, Nov 26, 2017 at 05:18:55PM -0500, Eric Sunshine wrote:
> On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov  wrote:
>> +int cmd_main(int argc, const char **argv)
>> +{
>> +   if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
>> +   printf("%zu", (ssize_t)(-20));
>> +
>> +   return 0;
> 
> Perhaps this should return 0 only if it gets the expected argument
> "(size_t)(-20)", and return an error otherwise.

Yes, makes sense.

> Rather than introducing a new 'test' program, would it be possible to
> get by with just using 'printf' from the shell?
> 
> % printf "%zu\n" -20
> 18446744073709551596

I thought about it, of course. But, I am not sure I can
exclude cases when the shell's printf uses 64-bit size_t and
git 32-bit one, or vise-versa. Same way, I cannot say it for
sure for any other software which I might use here instead
of the shell's printf. The only somewhat sure way would be
to use the same compiler, with same settings, which is used
for the production code.

I do not exclude possibility that my reasoning above is
wrong, either in general of specifically for git case. If
there are some examples where it is already used and the
risk of type size mismatch is prevented I could do it
similarly.

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Eric Sunshine]

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov  wrote:
> Add tests for cases:
>
> * CONTENT_LENGTH is set, script's stdin has more data.
>   (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
>   and fail. It does not seem to cause any performance issues with the default
>   value of GIT_HTTP_MAX_REQUEST_BUFFER.)
> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
>
> Signed-off-by: Max Kirillov 
> ---
> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
> @@ -0,0 +1,10 @@
> +int cmd_main(int argc, const char **argv)
> +{
> +   if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
> +   printf("%zu", (ssize_t)(-20));
> +
> +   return 0;

Perhaps this should return 0 only if it gets the expected argument
"(size_t)(-20)", and return an error otherwise.

> +}
> diff --git a/t/t5560-http-backend-noserver.sh 
> b/t/t5560-http-backend-noserver.sh
> @@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
> +test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
> +   NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" 
> "(size_t)(-20)"` &&

Rather than introducing a new 'test' program, would it be possible to
get by with just using 'printf' from the shell?

% printf "%zu\n" -20
18446744073709551596

> +   env \
> +   CONTENT_TYPE=application/x-git-upload-pack-request \
> +   QUERY_STRING=/repo.git/git-upload-pack \
> +   PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
> +   GIT_HTTP_EXPORT_ALL=TRUE \
> +   REQUEST_METHOD=POST \
> +   CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
> +   git http-backend /dev/null 2>err &&
> +   grep -q "fatal:.*CONTENT_LENGTH" err
> +'

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov  wrote:
> [...]
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
> [...]

A few small comments below; with the possible exception of one,
probably none worth a re-roll...

> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char 
> **out)
> +{
> +   unsigned char *buf = NULL;
> +   ssize_t cnt = 0;
> +
> +   if (max_request_buffer < req_len) {
> +   die("request was larger than our maximum size (%lu): %lu;"
> +   " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +   max_request_buffer,
> +   req_len);

Unsigned format conversion '%lu' doesn't seem correct for ssize_t.

> +   }
> +
> +   if (req_len <= 0) {
> +   *out = NULL;
> +   return 0;
> +   }
> +
> +   buf = xmalloc(req_len);
> +   cnt = read_in_full(fd, buf, req_len);
> +   if (cnt < 0) {
> +   free(buf);
> +   return -1;
> +   } else {
> +   *out = buf;
> +   return cnt;
> +   }

This could have been written:

if (cnt < 0) {
free(buf);
return -1;
}
*out = buf;
return cnt;

but not worth a re-roll.

> +}
> +
> +static ssize_t env_content_length(void)

The caller of this function doesn't care how the content length is
being determined -- whether it comes from an environment variable or
is computed some other way; it cares only about the result. Having
"env" in the name ties it to checking only the environment. A more
generic name, such as get_content_length(), would help to decouple the
API from the implementation.

Nevertheless, not worth a re-roll.

> +{
> +   ssize_t val = -1;
> +   const char *str = getenv("CONTENT_LENGTH");
> +
> +   if (str && !git_parse_ssize_t(str, ))

git_parse_ssize_t() does the right thing even when 'str' is NULL, so
this condition could be simplified (but not worth a re-roll and may
not improve clarity).

> +   die("failed to parse CONTENT_LENGTH: %s", str);
> +   return val;
> +}
> +
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +   ssize_t req_len = env_content_length();

Grabbing and parsing the value from the environment variable is
effectively a one-liner, so env_content_length() could be dropped
altogether, and instead (taking advantage of git_parse_ssize_t()'s
proper NULL-handling):

if (!git_parse_ssize_t(getenv(...), _len))
die(...);

Not worth a re-roll.

> +   if (req_len < 0)
> +   return read_request_eof(fd, out);
> +   else
> +   return read_request_fixed_len(fd, req_len, out);
> +}

Re: git status always modifies index?

[Johannes Schindelin]

Hi Peff,

On Sun, 26 Nov 2017, Jeff King wrote:

> On Sat, Nov 25, 2017 at 10:55:25PM +0100, Johannes Schindelin wrote:
> 
> > > Right, I went a little off track of your original point.
> > > 
> > > What I was trying to get at is that naming it "status --no-lock-index"
> > > would not be the same thing (even though with the current implementation
> > > it would behave the same). IOW, can we improve the documentation of
> > > "status" to point to make it easier to discover this use case.
> > 
> > I had the hunch that renaming the option (and moving it away from `git
> > status`, even if it is currently only affecting `git status` and even if
> > it will most likely be desirable to have the option to really only prevent
> > `git status` from writing .lock files) was an unfortunate decision (and
> > made my life as Git for Windows quite a bit harder than really necessary,
> > it cost me over one workday of a bug hunt, mainly due to a false flag
> > indicating `git rebase` to be the culprit). And I hinted at it, too.
> 
> I remain unconvinced that we have actually uncovered a serious problem.

You did not. A colleague of mine did. And it was a problem in Git for
Windows only, caused by the changes necessitated by yours (which even used
my tests, which made it easy for my conflict resolution to do the wrong
thing by removing my --no-lock-index test in favor of your
--no-optional-locks test, breaking --no-lock-index).

It cost me almost two work days, and a lot of hair. And all I meant by "I
hinted at it, too" was that I felt that something like that was coming
when I saw your variation of my patches making it into git/git's master.

This kind of stuff really throws my upstreaming back quite a bit, not only
due to lost time, but also due to the frustration with the caused
regressions.

Now, the report indicates that not only Git for Windows had a problem, but
that the new feature is unnecessarily unintuitive. I would even claim that
the --no-lock-index option (even if it does not say "--read-only") would
have made for a better user experience because it is at least in the
expected place: the `git status` man page.

> Somebody asked if Git could do a thing, and people pointed him to the
> right option.

If people have to ask on the mailing list even after reading the man
pages, that's a strong indicator that we could do better.

> That option is new in the latest release.

In Git, yes. In Git for Windows, no. And it worked beautifully in Git for
Windows before v2.15.0.

> > I really never understood why --no-optional-locks had to be introduced
> > when it did exactly the same as --no-lock-index, and when the latter has a
> > right to exist in the first place, even in the purely hypothetical case
> > that we teach --no-optional-locks to handle more cases than just `git
> > status`' writing of the index (and in essence, it looks like premature
> > optimization): it is a very concrete use case that a user may want `git
> > status` to refrain from even trying to write any file, as this thread
> > shows very eloquently.
> 
> Besides potentially handling more than just "git status",

... which is a premature optimization...

> it differs in one other way: it can be triggered via and is carried
> through the environment.

... which Git for Windows' --no-lock-index *also* had to do (think
submodules). We simply figured that out only after introducing the option,
therefore it was carried as an add-on commit, and the plan was to squash
it in before upstreaming (obviously!).

So I contest your claim. `--no-lock-index` must be propagated to callees
in the same way as the (still hypothetical) `--no-optional-locks` that
would cover more than just `git status`.

> > Maybe it is time to reintroduce --no-lock-index, and make
> > --no-optional-locks' functionality a true superset of --no-lock-index'.
> 
> I'm not against having a separate option for "never write to the
> repository".

Whoa, slow down. We already introduced the `--no-optional-locks` option
for a completely hypothetical use case covering more than just `git
status`, a use case that may very well never see the light of day. (At
least it was my undederstanding that the conjecture of something like that
maybe being needed by somebody some time in the future was the entire
reason tobutcher the --no-lock-index approach into a very different
looking --no-optional-locks that is much harder to find in the
documentation.)

Let's not introduce yet another option for a completely hypothetical use
case that may be even more theoretical.

> I think it's potentially different than "don't lock", as I
> mentioned earlier.

I don't see the need at all at the moemnt.

> Frankly I don't see much value in "--no-lock-index" if we already have
> "--no-optional-locks".

Funny. I did not (and still do not) see the need for renaming `git status
--no-lock-index` to `git --no-optional-locks status` (i.e. cluttering the
global option space for something that really only `git status` needs).

Hello...

[FROM: ANDREW LAWSON]

FROM: ANDREW LAWSON.

Dear Sir,


My name is Mr. Andrew Lawson. I work as a procurement assistant.  I got your 
contact details during my search for a reliable and neutral company or 
individual to partner with in the area of investment.

I need your assistance to manage an investment fund in a profitable business in 
your region with good Annual Return on Investment (AROI). I know that this mail 
might come to you as a surprise because we neither know each other nor have 
ever met but please accept it with an open and positive mind.

Details of the investment and funding will be furnished to you when I receive 
your response which will also facilitate a face to face meeting with the 
investors.

Yours Sincerely,

Mr.  Anthony Lawson

Re: [PATCH] Makefile: check that tcl/tk is installed

[Christian Couder]

On Sun, Nov 26, 2017 at 8:15 PM, Jeff King  wrote:
> On Tue, Nov 21, 2017 at 12:58:17AM +0100, Christian Couder wrote:
>
>> > Can you say more about where this comes up?
>>
>> The original discussion is:
>>
>> https://public-inbox.org/git/b6b12040-100f-5965-6dfd-344c84ddd...@teddy.ch/
>>
>> and here are discussions related to version 1 of this patch:
>>
>> https://public-inbox.org/git/20171115125200.17006-1-chrisc...@tuxfamily.org/
>>
>> As Peff mentions in the original discussion, at the Bloomberg Git
>> sprint, we saw someone struggling to compile Git, because of these
>> msgfmt and Tcl/Tk issues.
>
> Actually, I think we had the _opposite_ problem there.
>
> The main problem your patch fixes is that we may silently build a
> version of gitk/git-gui that do not work. The "make" process completes,
> but they refer to a non-existent "wish" tool, and running them will
> fail.
>
> That's potentially annoying if you wanted those tools. But if you didn't
> care about them in the first place, it's fine.

I think it's a bit better to not install the tools if you don't care about them.
So overall whether you care or not about them, there is still a bit of
improvement.

> The opposite problem is when you don't care about those tools, and they
> _do_ break the build. And then just to get the rest of Git built, you
> have to know about and set NO_TCLTK.
>
> AFAIK that only happens if you don't have msgfmt installed. Because then
> the gitk and git-gui Makefiles try to auto-fallback to implementing
> msgfmt in tcl _during the build_, and there a lack of "tclsh" will break
> the build.
>
> I think your patch does say "consider setting NO_TCLTK" in that case,
> which is an improvement.

Yeah, so my patch actually improve things in all the cases.

> But it might be nicer still if it Just Worked
> (either because we don't do tcl/tk by default, or because we respect
> NO_GETTEXT in the gitk/git-gui Makefiles, or because our msgfmt can
> fallback further to not even using tclsh).

Yeah, it might be nicer if it just worked, but as I already answered
in another thread, it could break some environments if we just stopped
installing gitk and git-gui by default.

About improving the way msgfmt is handled, I am not against it. In
fact I might even give it a try, but I think it is a separate issue,
and I don't want to mix those issues right now.

> So I'm not really against this patch, but IMHO it doesn't make the
> interesting case (you don't care about tcl and are just trying to build
> git for the first time) all that much better.

I agree that it doesn't solve all the issues, if you are trying to
build git for the first time, but I do think that it makes it easier.
If you don't have msgfmt, you get an error that is not so difficult to
debug.

> I do also wonder if we
> want to start putting these kind of run-time checks into the Makefile
> itself. That's kind of what autoconf is for.

I don't quite agree that autoconf is for that, and there are already
some checks in the Makefile.

> As much as I hate autoconf,
> is it the right advice for somebody who doesn't want to look at the
> Makefile knobs to do:
>
>   autoconf
>   ./configure
>   make
>
> ?

I don't think so. I think it is just easier to advice to do as most of
us do, and to just add a few checks in the Makefile to make it clear
which dependencies should be installed or which knob should be
tweaked.

> If there are deficiencies in configure.in (and I can well believe that
> there are), should we be fixing it there?

If most of us don't use autoconf, even if we fix the current
deficiencies, there could still be some a few years from now. While if
we add checks to the Makefile, there is a good chance that those who
change the Makefile will see the existing tests and add more if
necessary.

[PATCH v1 1/2] t/README: remove mention of adding copyright notices

[Thomas Gummerer]

We generally no longer include copyright notices in new test scripts.
However t/README still mentions it as something to include at the top of
every new script.

Remove that mention as it's outdated.

Signed-off-by: Thomas Gummerer 
---

I read through some parts of t/README, while working on the tests for
the worktree dwim patches, and noticed some things that are outdated/a
useful function that's not documented anywhere.  Here's a couple of
patches to fix that.

 t/README | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/README b/t/README
index 4b079e4494..448569b60e 100644
--- a/t/README
+++ b/t/README
@@ -332,13 +332,10 @@ Writing Tests
 -
 
 The test script is written as a shell script.  It should start
-with the standard "#!/bin/sh" with copyright notices, and an
+with the standard "#!/bin/sh", and an
 assignment to variable 'test_description', like this:
 
#!/bin/sh
-   #
-   # Copyright (c) 2005 Junio C Hamano
-   #
 
test_description='xxx test (option --frotz)
 
-- 
2.15.0.426.gb06021eeb

[PATCH v1 2/2] t/README: document test_cmp_rev

[Thomas Gummerer]

test_cmp_rev is a useful function that's used in quite a few test
scripts.  It is however not documented in t/README.  Document it.

Signed-off-by: Thomas Gummerer 
---
 t/README | 5 +
 1 file changed, 5 insertions(+)

diff --git a/t/README b/t/README
index 448569b60e..e867d1f8bd 100644
--- a/t/README
+++ b/t/README
@@ -674,6 +674,11 @@ library for your script to use.
 file.  This behaves like "cmp" but produces more
helpful output when the test is run with "-v" option.
 
+ - test_cmp_rev  
+
+   Check whether the  rev points to the same commit as the
+rev.
+
  - test_line_count (= | -lt | -ge | ...)  
 
Check whether a file has the length it is expected to.
-- 
2.15.0.426.gb06021eeb

[PATCH 1/2] trace: remove trace key normalization

[gennady . kupava]

From: Gennady Kupava 

Trace key normalization is not used, not strictly necessary,
complicates the code and would negatively affect compilation speed if
moved to header.

New trace_default_key key or existing separate marco could be used
instead of passing NULL as a key.

Signed-off-by: Gennady Kupava 
---
 trace.c | 24 
 trace.h |  4 +++-
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/trace.c b/trace.c
index cb1293ed3..d47ea28e8 100644
--- a/trace.c
+++ b/trace.c
@@ -24,26 +24,13 @@
 #include "cache.h"
 #include "quote.h"
 
-/*
- * "Normalize" a key argument by converting NULL to our trace_default,
- * and otherwise passing through the value. All caller-facing functions
- * should normalize their inputs in this way, though most get it
- * for free by calling get_trace_fd() (directly or indirectly).
- */
-static void normalize_trace_key(struct trace_key **key)
-{
-   static struct trace_key trace_default = { "GIT_TRACE" };
-   if (!*key)
-   *key = _default;
-}
+struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
const char *trace;
 
-   normalize_trace_key();
-
/* don't open twice */
if (key->initialized)
return key->fd;
@@ -81,8 +68,6 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
-   normalize_trace_key();
-
if (key->need_close)
close(key->fd);
key->fd = 0;
@@ -128,7 +113,6 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
-   normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
trace_disable(key);
@@ -167,13 +151,13 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (!prepare_trace_line(file, line, NULL, ))
+   if (!prepare_trace_line(file, line, _default_key, ))
return;
 
strbuf_vaddf(, format, ap);
 
sq_quote_argv(, argv, 0);
-   print_trace_line(NULL, );
+   print_trace_line(_default_key, );
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -215,7 +199,7 @@ void trace_printf(const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_vprintf_fl(NULL, 0, NULL, format, ap);
+   trace_vprintf_fl(NULL, 0, _default_key, format, ap);
va_end(ap);
 }
 
diff --git a/trace.h b/trace.h
index 179b249c5..24b32f8f4 100644
--- a/trace.h
+++ b/trace.h
@@ -11,6 +11,8 @@ struct trace_key {
unsigned int  need_close : 1;
 };
 
+extern struct trace_key trace_default_key;
+
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
 
 extern void trace_repo_setup(const char *prefix);
@@ -78,7 +80,7 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  */
 
 #define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
 
 #define trace_printf_key(key, ...) \
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-- 
2.14.1

[PATCH 2/2] trace: improve performance while category is disabled

[gennady . kupava]

From: Gennady Kupava 

- Do the check if the trace key is enabled sooner in call chain.
- Move just enough code from trace.c into trace.h header so all code
  necessary to determine that trace is disabled could be inlined to
  calling functions.

Signed-off-by: Gennady Kupava 
---
 trace.c |  3 +--
 trace.h | 58 --
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/trace.c b/trace.c
index d47ea28e8..b7530b51a 100644
--- a/trace.c
+++ b/trace.c
@@ -25,6 +25,7 @@
 #include "quote.h"
 
 struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
+struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
@@ -172,8 +173,6 @@ void trace_strbuf_fl(const char *file, int line, struct 
trace_key *key,
print_trace_line(key, );
 }
 
-static struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
-
 static void trace_performance_vprintf_fl(const char *file, int line,
 uint64_t nanos, const char *format,
 va_list ap)
diff --git a/trace.h b/trace.h
index 24b32f8f4..db10f2afe 100644
--- a/trace.h
+++ b/trace.h
@@ -14,6 +14,7 @@ struct trace_key {
 extern struct trace_key trace_default_key;
 
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
+extern struct trace_key trace_perf_key;
 
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(struct trace_key *key);
@@ -79,24 +80,42 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  * comma, but this is non-standard.
  */
 
-#define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
-
-#define trace_printf_key(key, ...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-
-#define trace_argv_printf(argv, ...) \
-   trace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)
-
-#define trace_strbuf(key, data) \
-   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data)
-
-#define trace_performance(nanos, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos, __VA_ARGS__)
-
-#define trace_performance_since(start, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, getnanotime() - (start), \
-__VA_ARGS__)
+#define trace_printf_key(key, ...) \
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,   \
+   __VA_ARGS__);   \
+   } while(0)
+
+#define trace_printf(...) trace_printf_key(_default_key, __VA_ARGS__)
+
+#define trace_argv_printf(argv, ...)   \
+   do {\
+   if (trace_pass_fl(_default_key))  \
+  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\
+   argv, __VA_ARGS__); \
+   } while(0)
+
+#define trace_strbuf(key, data)
\
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
+   } while(0)
+
+#define trace_performance(nanos, ...)  \
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\
+__VA_ARGS__);  \
+   } while(0)
+
+#define trace_performance_since(start, ...)\
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__,   \
+getnanotime() - (start),   \
+__VA_ARGS__);  \
+   } while(0)
 
 /* backend functions, use non-*fl macros instead */
 __attribute__((format (printf, 4, 5)))
@@ -110,6 +129,9 @@ extern void trace_strbuf_fl(const char *file, int line, 
struct trace_key *key,
 __attribute__((format (printf, 4, 5)))
 extern void trace_performance_fl(const char *file, int line,
 uint64_t nanos, const char *fmt, ...);
+inline int 

[PATCH v5 5/6] worktree: add --guess-remote flag to add subcommand

[Thomas Gummerer]

Currently 'git worktree add ' creates a new branch named after the
basename of the , that matches the HEAD of whichever worktree we
were on when calling "git worktree add ".

It's sometimes useful to have 'git worktree add  behave more like
the dwim machinery in 'git checkout ', i.e. check if the new
branch name uniquely matches the branch name of a remote-tracking
branch, and if so check out that branch and set the upstream to the
remote-tracking branch.

Add a new --guess-remote option that enables exactly that behaviour.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  7 +++
 builtin/worktree.c | 10 ++
 t/t2025-worktree-add.sh| 29 +
 3 files changed, 46 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 3044d305a6..a81cfb2229 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -115,6 +115,13 @@ OPTIONS
such as configuring sparse-checkout. See "Sparse checkout"
in linkgit:git-read-tree[1].
 
+--[no-]guess-remote::
+   With `add`, instead of creating a new branch from HEAD when
+   `` is not given, if there exists a tracking branch
+   in exactly one remote matching the basename of the path, base
+   the new branch on the remote-tracking branch, and mark the
+   remote-tracking branch as "upstream" from the new branch.
+
 --[no-]track::
When creating a new branch, if `` is a branch,
mark it as "upstream" from the new branch.  This is the
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7021d02585..15cb1600ee 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -343,6 +343,7 @@ static int add(int ac, const char **av, const char *prefix)
char *path;
const char *branch;
const char *opt_track = NULL;
+   int guess_remote = 0;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -355,6 +356,8 @@ static int add(int ac, const char **av, const char *prefix)
OPT_PASSTHRU(0, "track", _track, NULL,
 N_("set up tracking mode (see git-branch(1))"),
 PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
+   OPT_BOOL(0, "guess-remote", _remote,
+N_("try to match the new branch name with a 
remote-tracking branch")),
OPT_END()
};
 
@@ -389,6 +392,13 @@ static int add(int ac, const char **av, const char *prefix)
int n;
const char *s = worktree_basename(path, );
opts.new_branch = xstrndup(s, n);
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts.new_branch, );
+   if (remote)
+   branch = remote;
+   }
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 96ebc63d04..d25c774cb7 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -384,4 +384,33 @@ test_expect_success '"add"   dwims' '
)
 '
 
+test_expect_success 'git worktree add does not match remote' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git worktree add ../foo
+   ) &&
+   (
+   cd foo &&
+   test_must_fail git config "branch.foo.remote" &&
+   test_must_fail git config "branch.foo.merge" &&
+   ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+   )
+'
+
+test_expect_success 'git worktree add --guess-remote sets up tracking' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git worktree add --guess-remote ../foo
+   ) &&
+   (
+   cd foo &&
+   test_branch_upstream foo repo_a foo &&
+   test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+   )
+'
+
 test_done
-- 
2.15.0.426.gb06021eeb

[PATCH v5 2/6] worktree: add can be created from any commit-ish

[Thomas Gummerer]

Currently 'git worktree add' is documented to take an optional 
argument, which is checked out in the new worktree.  However it is more
generally possible to use a commit-ish as the optional argument, and
check that out into the new worktree.

Document that this is a possibility, as new users of git worktree add
might find it helpful.

Reported-by: Randall S. Becker 
Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index b472acc356..121895209d 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 
 [verse]
-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] 
 []
+'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] 
 []
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason ] 
 'git worktree prune' [-n] [-v] [--expire ]
@@ -45,14 +45,14 @@ specifying `--reason` to explain why the working tree is 
locked.
 
 COMMANDS
 
-add  []::
+add  []::
 
-Create `` and checkout `` into it. The new working directory
+Create `` and checkout `` into it. The new working directory
 is linked to the current repository, sharing everything except working
 directory specific files such as HEAD, index, etc. `-` may also be
-specified as ``; it is synonymous with `@{-1}`.
+specified as ``; it is synonymous with `@{-1}`.
 +
-If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
+If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename )` was specified.
 
@@ -84,25 +84,25 @@ OPTIONS
 
 -f::
 --force::
-   By default, `add` refuses to create a new working tree when ``
+   By default, `add` refuses to create a new working tree when 
`` is a branch name and
is already checked out by another working tree. This option overrides
that safeguard.
 
 -b ::
 -B ::
With `add`, create a new branch named `` starting at
-   ``, and check out `` into the new working tree.
-   If `` is omitted, it defaults to HEAD.
+   ``, and check out `` into the new working tree.
+   If `` is omitted, it defaults to HEAD.
By default, `-b` refuses to create a new branch if it already
exists. `-B` overrides this safeguard, resetting `` to
-   ``.
+   ``.
 
 --detach::
With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
in linkgit:git-checkout[1].
 
 --[no-]checkout::
-   By default, `add` checks out ``, however, `--no-checkout` can
+   By default, `add` checks out ``, however, `--no-checkout` 
can
be used to suppress checkout in order to make customizations,
such as configuring sparse-checkout. See "Sparse checkout"
in linkgit:git-read-tree[1].
-- 
2.15.0.426.gb06021eeb

[PATCH v5 1/6] checkout: factor out functions to new lib file

[Thomas Gummerer]

Factor the functions out, so they can be re-used from other places.  In
particular these functions will be re-used in builtin/worktree.c to make
git worktree add dwim more.

While there add some docs to the function.

Signed-off-by: Thomas Gummerer 
---
 Makefile   |  1 +
 builtin/checkout.c | 41 +
 checkout.c | 43 +++
 checkout.h | 13 +
 4 files changed, 58 insertions(+), 40 deletions(-)
 create mode 100644 checkout.c
 create mode 100644 checkout.h

diff --git a/Makefile b/Makefile
index e53750ca01..a80a8fcca9 100644
--- a/Makefile
+++ b/Makefile
@@ -759,6 +759,7 @@ LIB_OBJS += branch.o
 LIB_OBJS += bulk-checkin.o
 LIB_OBJS += bundle.o
 LIB_OBJS += cache-tree.o
+LIB_OBJS += checkout.o
 LIB_OBJS += color.o
 LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7d8bcc3833..ad8f94044c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "config.h"
+#include "checkout.h"
 #include "lockfile.h"
 #include "parse-options.h"
 #include "refs.h"
@@ -872,46 +873,6 @@ static int git_checkout_config(const char *var, const char 
*value, void *cb)
return git_xmerge_config(var, value, NULL);
 }
 
-struct tracking_name_data {
-   /* const */ char *src_ref;
-   char *dst_ref;
-   struct object_id *dst_oid;
-   int unique;
-};
-
-static int check_tracking_name(struct remote *remote, void *cb_data)
-{
-   struct tracking_name_data *cb = cb_data;
-   struct refspec query;
-   memset(, 0, sizeof(struct refspec));
-   query.src = cb->src_ref;
-   if (remote_find_tracking(remote, ) ||
-   get_oid(query.dst, cb->dst_oid)) {
-   free(query.dst);
-   return 0;
-   }
-   if (cb->dst_ref) {
-   free(query.dst);
-   cb->unique = 0;
-   return 0;
-   }
-   cb->dst_ref = query.dst;
-   return 0;
-}
-
-static const char *unique_tracking_name(const char *name, struct object_id 
*oid)
-{
-   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
-   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
-   cb_data.dst_oid = oid;
-   for_each_remote(check_tracking_name, _data);
-   free(cb_data.src_ref);
-   if (cb_data.unique)
-   return cb_data.dst_ref;
-   free(cb_data.dst_ref);
-   return NULL;
-}
-
 static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
diff --git a/checkout.c b/checkout.c
new file mode 100644
index 00..ac42630f74
--- /dev/null
+++ b/checkout.c
@@ -0,0 +1,43 @@
+#include "cache.h"
+#include "remote.h"
+#include "checkout.h"
+
+struct tracking_name_data {
+   /* const */ char *src_ref;
+   char *dst_ref;
+   struct object_id *dst_oid;
+   int unique;
+};
+
+static int check_tracking_name(struct remote *remote, void *cb_data)
+{
+   struct tracking_name_data *cb = cb_data;
+   struct refspec query;
+   memset(, 0, sizeof(struct refspec));
+   query.src = cb->src_ref;
+   if (remote_find_tracking(remote, ) ||
+   get_oid(query.dst, cb->dst_oid)) {
+   free(query.dst);
+   return 0;
+   }
+   if (cb->dst_ref) {
+   free(query.dst);
+   cb->unique = 0;
+   return 0;
+   }
+   cb->dst_ref = query.dst;
+   return 0;
+}
+
+const char *unique_tracking_name(const char *name, struct object_id *oid)
+{
+   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
+   cb_data.dst_oid = oid;
+   for_each_remote(check_tracking_name, _data);
+   free(cb_data.src_ref);
+   if (cb_data.unique)
+   return cb_data.dst_ref;
+   free(cb_data.dst_ref);
+   return NULL;
+}
diff --git a/checkout.h b/checkout.h
new file mode 100644
index 00..9980711179
--- /dev/null
+++ b/checkout.h
@@ -0,0 +1,13 @@
+#ifndef CHECKOUT_H
+#define CHECKOUT_H
+
+#include "cache.h"
+
+/*
+ * Check if the branch name uniquely matches a branch name on a remote
+ * tracking branch.  Return the name of the remote if such a branch
+ * exists, NULL otherwise.
+ */
+extern const char *unique_tracking_name(const char *name, struct object_id 
*oid);
+
+#endif /* CHECKOUT_H */
-- 
2.15.0.426.gb06021eeb

[PATCH v5 3/6] worktree: add --[no-]track option to the add subcommand

[Thomas Gummerer]

Currently 'git worktree add' sets up tracking branches if '' is
a remote tracking branch, and doesn't set them up otherwise, as is the
default for 'git branch'.

This may or may not be what the user wants.  Allow overriding this
behaviour with a --[no-]track flag that gets passed through to 'git
branch'.

We already respect branch.autoSetupMerge, as 'git worktree' just calls
'git branch' internally.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  6 +
 builtin/worktree.c |  8 +++
 t/t2025-worktree-add.sh| 51 ++
 3 files changed, 65 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 121895209d..15e58b18f7 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -107,6 +107,12 @@ OPTIONS
such as configuring sparse-checkout. See "Sparse checkout"
in linkgit:git-read-tree[1].
 
+--[no-]track::
+   When creating a new branch, if `` is a branch,
+   mark it as "upstream" from the new branch.  This is the
+   default if `` is a remote-tracking branch.  See
+   "--track" in linkgit:git-branch[1] for details.
+
 --lock::
Keep the working tree locked after creation. This is the
equivalent of `git worktree lock` after `git worktree add`,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index ed043d5f1c..ea9678cac8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -341,6 +341,7 @@ static int add(int ac, const char **av, const char *prefix)
const char *new_branch_force = NULL;
char *path;
const char *branch;
+   const char *opt_track = NULL;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -350,6 +351,9 @@ static int add(int ac, const char **av, const char *prefix)
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
OPT_BOOL(0, "lock", _locked, N_("keep the new working 
tree locked")),
+   OPT_PASSTHRU(0, "track", _track, NULL,
+N_("set up tracking mode (see git-branch(1))"),
+PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
OPT_END()
};
 
@@ -394,9 +398,13 @@ static int add(int ac, const char **av, const char *prefix)
argv_array_push(, "--force");
argv_array_push(, opts.new_branch);
argv_array_push(, branch);
+   if (opt_track)
+   argv_array_push(, opt_track);
if (run_command())
return -1;
branch = opts.new_branch;
+   } else if (opt_track) {
+   die(_("--[no-]track can only be used if a new branch is 
created"));
}
 
UNLEAK(path);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b5c47ac602..72e8b62927 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -313,5 +313,56 @@ test_expect_success 'checkout a branch under bisect' '
 test_expect_success 'rename a branch under bisect not allowed' '
test_must_fail git branch -M under-bisect bisect-with-new-name
 '
+# Is branch "refs/heads/$1" set to pull from "$2/$3"?
+test_branch_upstream () {
+   printf "%s\n" "$2" "refs/heads/$3" >expect.upstream &&
+   {
+   git config "branch.$1.remote" &&
+   git config "branch.$1.merge"
+   } >actual.upstream &&
+   test_cmp expect.upstream actual.upstream
+}
+
+test_expect_success '--track sets up tracking' '
+   test_when_finished rm -rf track &&
+   git worktree add --track -b track track master &&
+   test_branch_upstream track . master
+'
+
+# setup remote repository $1 and repository $2 with $1 set up as
+# remote.  The remote has two branches, master and foo.
+setup_remote_repo () {
+   git init $1 &&
+   (
+   cd $1 &&
+   test_commit $1_master &&
+   git checkout -b foo &&
+   test_commit upstream_foo
+   ) &&
+   git init $2 &&
+   (
+   cd $2 &&
+   test_commit $2_master &&
+   git remote add $1 ../$1 &&
+   git config remote.$1.fetch \
+   "refs/heads/*:refs/remotes/$1/*" &&
+   git fetch --all
+   )
+}
+
+test_expect_success '--no-track avoids setting up tracking' '
+   test_when_finished rm -rf repo_upstream repo_local foo &&
+   setup_remote_repo repo_upstream repo_local &&
+   (
+   cd repo_local &&
+   git worktree add --no-track -b foo ../foo repo_upstream/foo
+   ) &&
+   (
+   cd foo &&
+   test_must_fail git config 

[PATCH v5 6/6] add worktree.guessRemote config option

[Thomas Gummerer]

Some users might want to have the --guess-remote option introduced in
the previous commit on by default, so they don't have to type it out
every time they create a new worktree.

Add a config option worktree.guessRemote that allows users to configure
the default behaviour for themselves.

Signed-off-by: Thomas Gummerer 
---
 Documentation/config.txt   | 10 ++
 Documentation/git-worktree.txt |  3 +++
 builtin/worktree.c | 14 +-
 t/t2025-worktree-add.sh| 31 +++
 4 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f65fa9234..4966d90ebb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3425,3 +3425,13 @@ web.browser::
Specify a web browser that may be used by some commands.
Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
may use it.
+
+worktree.guessRemote::
+   With `add`, if no branch argument, and neither of `-b` nor
+   `-B` nor `--detach` are given, the command defaults to
+   creating a new branch from HEAD.  If `worktree.guessRemote` is
+   set to true, `worktree add` tries to find a remote-tracking
+   branch whose name uniquely matches the new branch name.  If
+   such a branch exists, it is checked out and set as "upstream"
+   for the new branch.  If no such match can be found, it falls
+   back to creating a new branch from the current HEAD.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index a81cfb2229..fd841886ef 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -121,6 +121,9 @@ OPTIONS
in exactly one remote matching the basename of the path, base
the new branch on the remote-tracking branch, and mark the
remote-tracking branch as "upstream" from the new branch.
++
+This can also be set up as the default behaviour by using the
+`worktree.guessRemote` config option.
 
 --[no-]track::
When creating a new branch, if `` is a branch,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 15cb1600ee..426aea8761 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -33,8 +33,19 @@ struct add_opts {
 
 static int show_only;
 static int verbose;
+static int guess_remote;
 static timestamp_t expire;
 
+static int git_worktree_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "worktree.guessremote")) {
+   guess_remote = git_config_bool(var, value);
+   return 0;
+   }
+
+   return 0;
+}
+
 static int prune_worktree(const char *id, struct strbuf *reason)
 {
struct stat st;
@@ -343,7 +354,6 @@ static int add(int ac, const char **av, const char *prefix)
char *path;
const char *branch;
const char *opt_track = NULL;
-   int guess_remote = 0;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -361,6 +371,8 @@ static int add(int ac, const char **av, const char *prefix)
OPT_END()
};
 
+   git_config(git_worktree_config, NULL);
+
memset(, 0, sizeof(opts));
opts.checkout = 1;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index d25c774cb7..6ce9b9c070 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -413,4 +413,35 @@ test_expect_success 'git worktree add --guess-remote sets 
up tracking' '
)
 '
 
+test_expect_success 'git worktree add with worktree.guessRemote sets up 
tracking' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git config worktree.guessRemote true &&
+   git worktree add ../foo
+   ) &&
+   (
+   cd foo &&
+   test_branch_upstream foo repo_a foo &&
+   test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+   )
+'
+
+test_expect_success 'git worktree --no-guess-remote option overrides config' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&
+   (
+   cd repo_b &&
+   git config worktree.guessRemote true &&
+   git worktree add --no-guess-remote ../foo
+   ) &&
+   (
+   cd foo &&
+   test_must_fail git config "branch.foo.remote" &&
+   test_must_fail git config "branch.foo.merge" &&
+   ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+   )
+'
+
 test_done
-- 
2.15.0.426.gb06021eeb

[PATCH v5 4/6] worktree: make add dwim

[Thomas Gummerer]

Currently 'git worktree add  ', errors out when 'branch'
is not a local branch.  It has no additional dwim'ing features that one
might expect.

Make it behave more like 'git checkout ' when the branch doesn't
exist locally, but a remote tracking branch uniquely matches the desired
branch name, i.e. create a new branch from the remote tracking branch
and set the upstream to the remote tracking branch.

As 'git worktree add' currently just dies in this situation, there are
no backwards compatibility worries when introducing this feature.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  8 
 builtin/worktree.c | 16 
 t/t2025-worktree-add.sh| 19 +++
 3 files changed, 43 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 15e58b18f7..3044d305a6 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -52,6 +52,14 @@ is linked to the current repository, sharing everything 
except working
 directory specific files such as HEAD, index, etc. `-` may also be
 specified as ``; it is synonymous with `@{-1}`.
 +
+If  is a branch name (call it `` and is not found,
+and neither `-b` nor `-B` nor `--detach` are used, but there does
+exist a tracking branch in exactly one remote (call it ``)
+with a matching name, treat as equivalent to
+
+$ git worktree add --track -b   /
+
++
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename )` was specified.
diff --git a/builtin/worktree.c b/builtin/worktree.c
index ea9678cac8..7021d02585 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "checkout.h"
 #include "config.h"
 #include "builtin.h"
 #include "dir.h"
@@ -390,6 +391,21 @@ static int add(int ac, const char **av, const char *prefix)
opts.new_branch = xstrndup(s, n);
}
 
+   if (ac == 2 && !opts.new_branch && !opts.detach) {
+   struct object_id oid;
+   struct commit *commit;
+   const char *remote;
+
+   commit = lookup_commit_reference_by_name(branch);
+   if (!commit) {
+   remote = unique_tracking_name(branch, );
+   if (remote) {
+   opts.new_branch = branch;
+   branch = remote;
+   }
+   }
+   }
+
if (opts.new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 72e8b62927..96ebc63d04 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -365,4 +365,23 @@ test_expect_success '--no-track avoids setting up 
tracking' '
)
 '
 
+test_expect_success '"add"   fails' '
+   test_must_fail git worktree add foo non-existent
+'
+
+test_expect_success '"add"   dwims' '
+   test_when_finished rm -rf repo_upstream repo_dwim foo &&
+   setup_remote_repo repo_upstream repo_dwim &&
+   git init repo_dwim &&
+   (
+   cd repo_dwim &&
+   git worktree add ../foo foo
+   ) &&
+   (
+   cd foo &&
+   test_branch_upstream foo repo_upstream foo &&
+   test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
+   )
+'
+
 test_done
-- 
2.15.0.426.gb06021eeb

[PATCH v5 0/6] make git worktree add dwim more

[Thomas Gummerer]

The previous rounds can be found at
https://public-inbox.org/git/20171112134305.3949-1-t.gumme...@gmail.com/,
https://public-inbox.org/git/20171118181103.28354-1-t.gumme...@gmail.com/,
https://public-inbox.org/git/20171118224706.13810-1-t.gumme...@gmail.com/ and
https://public-inbox.org/git/2017113020.2780-1-t.gumme...@gmail.com/.

Thanks Junio for the review of the previous round and Randall for the
suggestion of documenting that git worktree add can take a commit-ish,
not just a branch.

The main changes in this round are hiding the new behaviour for 'git
worktree ' behind a flag, and adding a config option to turn the
new behaviour on by default.  It's also no longer relying on the
--[no]-track flag, but using a new --[no-]guess-remote flag instead.

Interdiff between this and the previous round below:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f65fa9234..4966d90ebb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3425,3 +3425,13 @@ web.browser::
Specify a web browser that may be used by some commands.
Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
may use it.
+
+worktree.guessRemote::
+   With `add`, if no branch argument, and neither of `-b` nor
+   `-B` nor `--detach` are given, the command defaults to
+   creating a new branch from HEAD.  If `worktree.guessRemote` is
+   set to true, `worktree add` tries to find a remote-tracking
+   branch whose name uniquely matches the new branch name.  If
+   such a branch exists, it is checked out and set as "upstream"
+   for the new branch.  If no such match can be found, it falls
+   back to creating a new branch from the current HEAD.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index abc8f1f50d..fd841886ef 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 
 [verse]
-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] 
 []
+'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] 
 []
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason ] 
 'git worktree prune' [-n] [-v] [--expire ]
@@ -45,33 +45,24 @@ specifying `--reason` to explain why the working tree is 
locked.
 
 COMMANDS
 
-add  []::
+add  []::
 
-Create `` and checkout `` into it. The new working directory
+Create `` and checkout `` into it. The new working directory
 is linked to the current repository, sharing everything except working
 directory specific files such as HEAD, index, etc. `-` may also be
-specified as ``; it is synonymous with `@{-1}`.
+specified as ``; it is synonymous with `@{-1}`.
 +
-If  is not found, and neither `-b` nor `-B` nor `--detach` are
-used, but there does exist a tracking branch in exactly one remote
-(call it ) with a matching name, treat as equivalent to
+If  is a branch name (call it `` and is not found,
+and neither `-b` nor `-B` nor `--detach` are used, but there does
+exist a tracking branch in exactly one remote (call it ``)
+with a matching name, treat as equivalent to
 
 $ git worktree add --track -b   /
 
 +
-If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, if there exists a tracking branch in exactly
-one remote (call it ``) matching the basename of the path
-(call it ``), treat it as equivalent to
-
-$ git worktree add --track -b   /
-
-If no tracking branch exists in exactly one remote, `` is
-created based on HEAD, as if `-b $(basename )` was specified.
-+
-To disable the behaviour of trying to match the basename of  to
-a remote, and always create a new branch from HEAD, the `--no-track`
-flag can be passed to `git worktree add`.
+If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
+then, as a convenience, a new branch based at HEAD is created automatically,
+as if `-b $(basename )` was specified.
 
 list::
 
@@ -101,34 +92,44 @@ OPTIONS
 
 -f::
 --force::
-   By default, `add` refuses to create a new working tree when ``
+   By default, `add` refuses to create a new working tree when 
`` is a branch name and
is already checked out by another working tree. This option overrides
that safeguard.
 
 -b ::
 -B ::
With `add`, create a new branch named `` starting at
-   ``, and check out `` into the new working tree.
-   If `` is omitted, it defaults to HEAD.
+   ``, and check out `` into the new working tree.
+   If `` is omitted, it defaults to HEAD.
By default, `-b` refuses to create a new branch if it already
exists. `-B` overrides this safeguard, resetting `` to
-   ``.
+   ``.
 
 --detach::
With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
in linkgit:git-checkout[1].
 
 --[no-]checkout::
-   By default, `add` checks out ``, however, 

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sun, Nov 26, 2017 at 06:38:06PM +0900, Junio C Hamano wrote:
> Max Kirillov  writes:
>> +static ssize_t env_content_length()
> 
> We need s/length()/length(void)/; though.

thanks, fixed it

[PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

v6:

Do not implement generic git_env_ssize_t(), instead export git_parse_ssize_t() 
from config.c
and hardcode the rest.

Florian Manschwetus (1):
  http-backend: respect CONTENT_LENGTH as specified by rfc3875

Max Kirillov (1):
  t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

 Makefile |  1 +
 config.c |  2 +-
 config.h |  1 +
 http-backend.c   | 50 +++-
 t/helper/test-print-values.c | 10 
 t/t5560-http-backend-noserver.sh | 30 
 6 files changed, 92 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-print-values.c

-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

From: Florian Manschwetus 

From: Florian Manschwetus 
Date: Wed, 30 Mar 2016 10:54:21 +0200

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.

Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.

Signed-off-by: Florian Manschwetus 
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov 
---
 config.c   |  2 +-
 config.h   |  1 +
 http-backend.c | 50 +-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 231f9a750b..d3ec14ab74 100644
--- a/config.c
+++ b/config.c
@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
return 1;
 }
 
-static int git_parse_ssize_t(const char *value, ssize_t *ret)
+int git_parse_ssize_t(const char *value, ssize_t *ret)
 {
intmax_t tmp;
if (!git_parse_signed(value, , 
maximum_signed_value_of_type(ssize_t)))
diff --git a/config.h b/config.h
index 0352da117b..46a4989def 100644
--- a/config.h
+++ b/config.h
@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);
 extern int config_with_options(config_fn_t fn, void *,
   struct git_config_source *config_source,
   const struct config_options *opts);
+extern int git_parse_ssize_t(const char *, ssize_t *);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_parse_maybe_bool(const char *);
 extern int git_config_int(const char *, const char *);
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..af7dd00d70 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf 
*hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
}
 }
 
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char 
**out)
+{
+   unsigned char *buf = NULL;
+   ssize_t cnt = 0;
+
+   if (max_request_buffer < req_len) {
+   die("request was larger than our maximum size (%lu): %lu;"
+   " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+   max_request_buffer,
+   req_len);
+   }
+
+   if (req_len <= 0) {
+   *out = NULL;
+   return 0;
+   }
+
+   buf = xmalloc(req_len);
+   cnt = read_in_full(fd, buf, req_len);
+   if (cnt < 0) {
+   free(buf);
+   return -1;
+   } else {
+   *out = buf;
+   return cnt;
+   }
+}
+
+static ssize_t env_content_length(void)
+{
+   ssize_t val = -1;
+   const char *str = getenv("CONTENT_LENGTH");
+
+   if (str && !git_parse_ssize_t(str, ))
+   die("failed to parse CONTENT_LENGTH: %s", str);
+   return val;
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+   ssize_t req_len = env_content_length();
+
+   if (req_len < 0)
+   return read_request_eof(fd, out);
+   else
+   return read_request_fixed_len(fd, req_len, out);
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
git_zstream stream;
-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

Add tests for cases:

* CONTENT_LENGTH is set, script's stdin has more data.
  (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
  and fail. It does not seem to cause any performance issues with the default
  value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

Signed-off-by: Max Kirillov 
---
 Makefile |  1 +
 t/helper/test-print-values.c | 10 ++
 t/t5560-http-backend-noserver.sh | 30 ++
 3 files changed, 41 insertions(+)
 create mode 100644 t/helper/test-print-values.c

diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 00..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include 
+#include 
+
+int cmd_main(int argc, const char **argv)
+{
+   if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+   printf("%zu", (ssize_t)(-20));
+
+   return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
expect_aliased 1 //domain/data.txt
 '
 
+# overrides existing definition for further cases
+run_backend() {
+   CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+   ( echo "$2" && cat /dev/zero ) |
+   QUERY_STRING="${1#*[?]}" \
+   PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+   git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+   config http.uploadpack true &&
+   GET info/refs?service=git-upload-pack "200 OK"  &&
+   ! grep "fatal:.*" act.err &&
+   POST git-upload-pack  "200 OK" &&
+   ! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+   NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" 
"(size_t)(-20)"` &&
+   env \
+   CONTENT_TYPE=application/x-git-upload-pack-request \
+   QUERY_STRING=/repo.git/git-upload-pack \
+   PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+   GIT_HTTP_EXPORT_ALL=TRUE \
+   REQUEST_METHOD=POST \
+   CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+   git http-backend /dev/null 2>err &&
+   grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
 test_done
-- 
2.11.0.1122.gc3fec58.dirty

Re: git status always modifies index?

[Jeff King]

On Sun, Nov 26, 2017 at 12:32:13PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > What I was trying to get at is that naming it "status --no-lock-index"
> > would not be the same thing (even though with the current implementation
> > it would behave the same). IOW, can we improve the documentation of
> > "status" to point to make it easier to discover this use case.
> 
> Yeah, the name is unfortunate. 
> 
> What the end user really wants to see, I suspect, is a "--read-only"
> option that applies to any filesystem entity and to any command, in
> the context of this thread, and also in the original discussion that
> led to the introduction of that option.

I'm not sure I agree. Lockless writes are actually fine for the original
use case of --no-optional-locks (which is a process for the same user
that just happens to run in the background). I can buy the distinction
between that and "--read-only" as premature optimization, though, since
it's not common for most operations to do non-locking writes (pretty
much object writes are the only thing, and most "semantically read-only"
operations like status or diff do not write any objects).

So there's very little lost by people in the first boat saying
"--read-only".

-Peff

Re: git status always modifies index?

[Jeff King]

On Sat, Nov 25, 2017 at 10:55:25PM +0100, Johannes Schindelin wrote:

> > Right, I went a little off track of your original point.
> > 
> > What I was trying to get at is that naming it "status --no-lock-index"
> > would not be the same thing (even though with the current implementation
> > it would behave the same). IOW, can we improve the documentation of
> > "status" to point to make it easier to discover this use case.
> 
> I had the hunch that renaming the option (and moving it away from `git
> status`, even if it is currently only affecting `git status` and even if
> it will most likely be desirable to have the option to really only prevent
> `git status` from writing .lock files) was an unfortunate decision (and
> made my life as Git for Windows quite a bit harder than really necessary,
> it cost me over one workday of a bug hunt, mainly due to a false flag
> indicating `git rebase` to be the culprit). And I hinted at it, too.

I remain unconvinced that we have actually uncovered a serious problem.
Somebody asked if Git could do a thing, and people pointed him to the
right option. That option is new in the latest release. So it is
entirely plausible to me that the new option is just fine and:

  1. We could adjust the documentation to cross-reference from
 git-status.

  2. Now that the new option exists, informal documentation will start
 to mention it. Including this thread in the mailing list archive,
 and the stack overflow thread that was linked.

> I really never understood why --no-optional-locks had to be introduced
> when it did exactly the same as --no-lock-index, and when the latter has a
> right to exist in the first place, even in the purely hypothetical case
> that we teach --no-optional-locks to handle more cases than just `git
> status`' writing of the index (and in essence, it looks like premature
> optimization): it is a very concrete use case that a user may want `git
> status` to refrain from even trying to write any file, as this thread
> shows very eloquently.

Besides potentially handling more than just "git status", it differs in
one other way: it can be triggered via and is carried through the
environment.

> Maybe it is time to reintroduce --no-lock-index, and make
> --no-optional-locks' functionality a true superset of --no-lock-index'.

I'm not against having a separate option for "never write to the
repository". I think it's potentially different than "don't lock", as I
mentioned earlier. Frankly I don't see much value in "--no-lock-index"
if we already have "--no-optional-locks". But I figured you would carry
"status --no-lock-index" forever in Git for Windows anyway (after all,
if you remove it now, you're breaking compatibility for existing users).

-Peff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Jeff King]

On Tue, Nov 21, 2017 at 12:58:17AM +0100, Christian Couder wrote:

> > Can you say more about where this comes up?
> 
> The original discussion is:
> 
> https://public-inbox.org/git/b6b12040-100f-5965-6dfd-344c84ddd...@teddy.ch/
> 
> and here are discussions related to version 1 of this patch:
> 
> https://public-inbox.org/git/20171115125200.17006-1-chrisc...@tuxfamily.org/
> 
> As Peff mentions in the original discussion, at the Bloomberg Git
> sprint, we saw someone struggling to compile Git, because of these
> msgfmt and Tcl/Tk issues.

Actually, I think we had the _opposite_ problem there.

The main problem your patch fixes is that we may silently build a
version of gitk/git-gui that do not work. The "make" process completes,
but they refer to a non-existent "wish" tool, and running them will
fail.

That's potentially annoying if you wanted those tools. But if you didn't
care about them in the first place, it's fine.

The opposite problem is when you don't care about those tools, and they
_do_ break the build. And then just to get the rest of Git built, you
have to know about and set NO_TCLTK.

AFAIK that only happens if you don't have msgfmt installed. Because then
the gitk and git-gui Makefiles try to auto-fallback to implementing
msgfmt in tcl _during the build_, and there a lack of "tclsh" will break
the build.

I think your patch does say "consider setting NO_TCLTK" in that case,
which is an improvement. But it might be nicer still if it Just Worked
(either because we don't do tcl/tk by default, or because we respect
NO_GETTEXT in the gitk/git-gui Makefiles, or because our msgfmt can
fallback further to not even using tclsh).

So I'm not really against this patch, but IMHO it doesn't make the
interesting case (you don't care about tcl and are just trying to build
git for the first time) all that much better. I do also wonder if we
want to start putting these kind of run-time checks into the Makefile
itself. That's kind of what autoconf is for. As much as I hate autoconf,
is it the right advice for somebody who doesn't want to look at the
Makefile knobs to do:

  autoconf
  ./configure
  make

?

If there are deficiencies in configure.in (and I can well believe that
there are), should we be fixing it there?

-Peff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Christian Couder]

On Sun, Nov 26, 2017 at 6:43 PM, Ramsay Jones
 wrote:
>
>
> On 26/11/17 14:00, Christian Couder wrote:
>> On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano  wrote:
>>> Christian Couder  writes:
>>>
 On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
  wrote:
> By default running `make install` in the root directory of the
> project will set TCLTK_PATH to `wish` and then go into the "git-gui"
> and "gitk-git" sub-directories to build and install these 2
> sub-projects.

 Has this patch fallen through the cracks or is there an unresolved issue?
>>>
>>> I had an impression that the conclusion was that the existing error
>>> message at runtime already does an adequate job and there is no
>>> issue to be addressed by this patch.  Am I mistaken?
>>
>> This patch is mostly about what happens at the build step. Its goal is
>> not much to improve what happens at runtime, though that is improved a
>> bit too. If the build step was good enough, then I would agree that
>> what happens at run time is adequate.
>>
>> Let's consider only people installing git using "make install" to use
>> it on their machine, as I think I already discussed the case of
>> packagers and added the BYPASS_TCLTK_CHECK variable for them.
>>
>
> I haven't been following this thread too closely, but I have the
> feeling that the best course of action is to simply not fall back
> to using a tcl version of msgfmt in the first place!. ;-)

Well, another possibility would be to try to use the tcl version of
msgfmt in the build of git itself if msgfmt is not available.
This way the build behavior of git and git-gui could be similar.

> If a given platform does not have gettext/msgfmt, then you just
> don't get an i18n-ed version of git. (no need for BYPASS_ ...).

Right now without gettext/msgfmt you get an error unless you set
NO_GETTEXT. If we try to use the tcl version of msgfmt in the build of
git itself, then you could still get an i18n-ed version of git if you
have Tcl/Tk.

But anyway even if this is related, I think it is a different issue.

> Am I missing something?

I still think it is not the best outcome to just install git-gui and
gitk by default when Tcl/Tk is not installed. In general it is best to
fix potential errors at build time rather than at run time (even if
the run time error is adequate).

Re: [PATCH] Makefile: check that tcl/tk is installed

[Ramsay Jones]

On 26/11/17 14:00, Christian Couder wrote:
> On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano  wrote:
>> Christian Couder  writes:
>>
>>> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
>>>  wrote:
 By default running `make install` in the root directory of the
 project will set TCLTK_PATH to `wish` and then go into the "git-gui"
 and "gitk-git" sub-directories to build and install these 2
 sub-projects.
>>>
>>> Has this patch fallen through the cracks or is there an unresolved issue?
>>
>> I had an impression that the conclusion was that the existing error
>> message at runtime already does an adequate job and there is no
>> issue to be addressed by this patch.  Am I mistaken?
> 
> This patch is mostly about what happens at the build step. Its goal is
> not much to improve what happens at runtime, though that is improved a
> bit too. If the build step was good enough, then I would agree that
> what happens at run time is adequate.
> 
> Let's consider only people installing git using "make install" to use
> it on their machine, as I think I already discussed the case of
> packagers and added the BYPASS_TCLTK_CHECK variable for them.
> 

I haven't been following this thread too closely, but I have the
feeling that the best course of action is to simply not fall back
to using a tcl version of msgfmt in the first place!. ;-)

If a given platform does not have gettext/msgfmt, then you just
don't get an i18n-ed version of git. (no need for BYPASS_ ...).

Am I missing something?

ATB,
Ramsay Jones

Re: [PATCH] Makefile: check that tcl/tk is installed

[Christian Couder]

On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
>>  wrote:
>>> By default running `make install` in the root directory of the
>>> project will set TCLTK_PATH to `wish` and then go into the "git-gui"
>>> and "gitk-git" sub-directories to build and install these 2
>>> sub-projects.
>>
>> Has this patch fallen through the cracks or is there an unresolved issue?
>
> I had an impression that the conclusion was that the existing error
> message at runtime already does an adequate job and there is no
> issue to be addressed by this patch.  Am I mistaken?

This patch is mostly about what happens at the build step. Its goal is
not much to improve what happens at runtime, though that is improved a
bit too. If the build step was good enough, then I would agree that
what happens at run time is adequate.

Let's consider only people installing git using "make install" to use
it on their machine, as I think I already discussed the case of
packagers and added the BYPASS_TCLTK_CHECK variable for them.

When those users run "make install", let's suppose they don't have
Tcl/Tk installed. (If it is already installed my patch changes
nothing.)
Let's also suppose that NO_TCLTK is not set. (If it is set my patch
changes nothing.)

Then there are 2 cases:

- msgfmt is already installed

Without my patch, the "make install" step works and installs git,
git-gui and gitk. But the user might not want to have git-gui and gitk
installed in the first place. As Jeff says there are a lot of other
graphical tools, that are more advanced these days, so there is a big
chance that they just forgot to set NO_TCLTK or just didn't know about
this variable. Now if they actually want to use git-gui and gitk, they
will get an error at run time (which is adequate) and they will have
to install Tck/Tk then before they can git-gui and gitk.

With my patch, the "make install" step fails right away telling them
to either set NO_TCLTK or install Tcl/Tk. If they don't want to use
git-gui and gitk, they will set NO_TCLTK and then run "make install"
again and things will be exactly as they want. If they do want to use
git-gui and gitk, they will install Tcl/Tk and then run "make install"
again and then things will be exactly as they want and there will be
no error at runtime.

So my opinion is that whether they actually want to use git-gui and
gitk or not, forcing them to decide is probably a good thing because
it ensures that when "make install" succeeds things are exactly how
they want them to be. The only case when it might not be a good thing
is if they don't know yet if they will actually want to use gitk and
git-gui. But even in this case, which is not the most common, they are
not much worse.

- msgfmt is not installed

Without my patch, the "make install" step produces an error message
while building git-gui. Debugging and fixing the error is quite
difficult especially for new comers. They will have to either set
NO_GETTEXT or install gettext, and to either set NO_TCLTK or to
install Tcl/Tk before the build can fully succeed.

With my patch, the "make install" step fails right away telling them
to either set NO_TCLTK or install Tcl/Tk. Then the build will fail
again and they will have to either set NO_GETTEXT or install gettext,
but this error is easier to understand and to fix than without my
patch.

[PATCH v2 1/1] convert: tighten the safe autocrlf handling

[tboegi]

From: Torsten Bögershausen 

When a text file had been commited with CRLF and the file is commited
again, the CRLF are kept if .gitattributs has "text=auto".
This is done by analyzing the content of the blob stored in the index:
If a '\r' is found, Git assumes that the blob was commited with CRLF.

The simple search for a '\r' does not always work as expected:
A file is encoded in UTF-16 with CRLF and commited. Git treats it as binary.
Now the content is converted into UTF-8. At the next commit Git treats the
file as text, the CRLF should be converted into LF, but isn't.

Replace has_cr_in_index() with has_crlf_in_index(). When no '\r' is found,
0 is returned directly, this is the most common case.
If a '\r' is found, the content is analyzed more deeply.

Reported-By: Ashish Negi 
Signed-off-by: Torsten Bögershausen 
---

Changes against v1:
- change "if (crp && (crp[1] == '\n'))" to "if (crp)"
  (Thanks Eric. The new patch is more straightforward, and no risk to
   read out of data)
- Remove the "Solution:" in the commit message

Note:
  The original function has_cr_in_index() is from this commit:
c4805393d73 (Finn Arne Gangstad   2010-05-12 00:37:57 +0200  225)
  And has this info:
>Change autocrlf to not do any conversions to files that in the
>repository already contain a CR. git with autocrlf set will never
>create such a file, or change a LF only file to contain CRs, so the
>(new) assumption is that if a file contains a CR, it is intentional,
>and autocrlf should not change that.
  So the original assumption was slightly optimistic (but did work in 7 years)



convert.c| 19 +
 t/t0027-auto-crlf.sh | 76 
 2 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/convert.c b/convert.c
index 20d7ab67bd..1a41a48e15 100644
--- a/convert.c
+++ b/convert.c
@@ -220,18 +220,27 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
}
 }
 
-static int has_cr_in_index(const struct index_state *istate, const char *path)
+static int has_crlf_in_index(const struct index_state *istate, const char 
*path)
 {
unsigned long sz;
void *data;
-   int has_cr;
+   const char *crp;
+   int has_crlf = 0;
 
data = read_blob_data_from_index(istate, path, );
if (!data)
return 0;
-   has_cr = memchr(data, '\r', sz) != NULL;
+
+   crp = memchr(data, '\r', sz);
+   if (crp) {
+   unsigned int ret_stats;
+   ret_stats = gather_convert_stats(data, sz);
+   if (!(ret_stats & CONVERT_STAT_BITS_BIN) &&
+   (ret_stats & CONVERT_STAT_BITS_TXT_CRLF))
+   has_crlf = 1;
+   }
free(data);
-   return has_cr;
+   return has_crlf;
 }
 
 static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
@@ -290,7 +299,7 @@ static int crlf_to_git(const struct index_state *istate,
 * cherry-pick.
 */
if ((checksafe != SAFE_CRLF_RENORMALIZE) &&
-   has_cr_in_index(istate, path))
+   has_crlf_in_index(istate, path))
convert_crlf_into_lf = 0;
}
if ((checksafe == SAFE_CRLF_WARN ||
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 68108d956a..0af35cfb1f 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -43,19 +43,31 @@ create_gitattributes () {
} >.gitattributes
 }
 
-create_NNO_files () {
+# Create 2 sets of files:
+# The NNO files are "Not NOrmalized in the repo. We use CRLF_mix_LF and store
+#   it under different names for the different test cases, see ${pfx}
+#   Depending on .gitattributes they are normalized at the next commit (or not)
+# The MIX files have different contents in the repo.
+#   Depending on its contents, the "new safer autocrlf" may kick in.
+create_NNO_MIX_files () {
for crlf in false true input
do
for attr in "" auto text -text
do
for aeol in "" lf crlf
do
-   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf} &&
cp CRLF_mix_LF ${pfx}_LF.txt &&
cp CRLF_mix_LF ${pfx}_CRLF.txt &&
cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
+   cp CRLF_mix_LF ${pfx}_CRLF_nul.txt &&
+   pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf} &&
+   cp LF  ${pfx}_LF.txt &&
+   cp CRLF${pfx}_CRLF.txt &&
+   

Re: Problem installing Git

[Igor Djordjevic]

Hi Johannes,

On 25/11/2017 23:16, Johannes Schindelin wrote:
> 
> > [ +Cc:  Git for Windows mailing list ]
> 
> I have no idea why it claimed that that group does not exist, the
> email address looks correct to me.

I suspected the culprit was me not being a member of the group, where 
the group requires it for (initial?) posting (but you already noticed 
that other e-mail about it there[2], this is more a follow-up for 
anyone here, still wondering).

> > p.s. Note the existence of "Git for Windows"[1] specific mailing
> > list as well, where this issue might belong better.
> >
> > [1] git-for-wind...@googlegroups.com
> 
> I think a much better place is the Git for Windows bug tracker (if
> you ever wonder where the bug tracker is, or the home page, or the
> repository or the FAQ, there are links in the upper left of the
> release notes -- which probably nobody reads, even if I really try to
> make them worth the while -- and which you can find in C:\Program
> Files\Git\ReleaseNotes.html if you closed the tab after installing
> Git for Windows).
> 
> And indeed, there is already a ticket for this issue:
> https://github.com/git-for-windows/git/issues/1074

I myself am aware of the tracker, just that I didn`t really 
anticipate this as a Git for Windows bug, seeing various mentions 
around the net pointing to Inno Setup (being in use here as well).

That said, I see the logic in thinking the other way around, too, and 
possibly addressing it somehow inside Git for Windows, in spite of 
what happens on Inno Setup side. At least working around it a bit 
better, if possible.

I can remember to keep an eye on the tracker more often, just in 
case, as that would have clearly helped here. Thanks for the heads-up.

Regards, Buga

[2] https://groups.google.com/d/msg/git-for-windows/0hn7Th4uGZk/oJTdmXa5AgAJ

FUND DONATION US$25MILLION

[adrian]

US$25 Million Has Been Granted To You As A Donation Visit 
www.bbc.co.uk/news/uk-england- 19254228 For Details  Send Us Your Name, Home 
Address And Phone Numbers,
 text us at ( adrian.payment2...@yahoo.com ) For More Information.

Best Regard:
Mr. Adrian and Gillian Bayford
FUND DONATION US$25MILLION

Re: [PATCH v4 4/4] worktree: make add dwim

[Thomas Gummerer]

On 11/26, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Of course that assumes that it's used directly, not in scripts, and
> > that users will actually read the output of the command when they
> > invoke it.  Maybe these are not safe assumptions to make though, and
> > we'd rather not have this on by default then.
> 
> None of these is a safe assumption to make.  With a transition plan,
> we could change the default eventually, but if this feature is already
> used by real users, the proposed change without any transition plan
> will hurt them and make them hate us, I am afraid.

Fair enough, I definitely wouldn't want users to hate us.  I guess
I'll add this to the list of things that I'd like to change when git
3.0 comes along.

How about once we make the decision when git 3.0 happens, adding a
warning:

warning: The current behaviour of creating a branch from HEAD when
no  is given will change in git 3.0, to try and match the
new branch name to a remote-tracking branch.  To get rid of this
warning in the meantime, set git.guessRemote to true or false,
depending on whether you'd like to enable the new behaviour now,
or would like to keep the existing behaviour. (see also
--guess-remote in man git-worktree)

or something along the lines, and once we decide to release git 3.0 we
flip the default?  The warning would be displayed whenever the new
behaviour would be invoked, but no config option or flag is set.

Either way I'm going to add a flag and a config option to hide this
behaviour behind for now.

Thanks!

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov  writes:

> I'm afraid I did not get the reasonsing and not fully the
> desired change. Is this http-backend code change (compared
> to the last patch) what you mean?

Exactly.  

The fact that the parsed string happens to come from CONTENT_LENGTH
environment variable is http's business, not parsers for various
types of values that come in the form of strings.

> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t 
> req_len, unsigned char **o
>   }
>  }
>  
> +static ssize_t env_content_length()

We need s/length()/length(void)/; though.

> +{
> + const char *str = getenv("CONTENT_LENGTH");
> + ssize_t val = -1;
> + if (str && !git_parse_ssize_t(str, ))
> + die("failed to parse CONTENT_LENGTH: %s", str);
> + return val;
> +}
> +
>  static ssize_t read_request(int fd, unsigned char **out)
>  {
> - ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
> + ssize_t req_len = env_content_length();
>   if (req_len < 0)
>   return read_request_eof(fd, out);
>   else

Re: git status always modifies index?

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> What I was trying to get at is that naming it "status --no-lock-index"
>> would not be the same thing (even though with the current implementation
>> it would behave the same). IOW, can we improve the documentation of
>> "status" to point to make it easier to discover this use case.
>
> Yeah, the name is unfortunate. 
>
> What the end user really wants to see, I suspect, is a "--read-only"
> option that applies to any filesystem entity and to any command, in
> the context of this thread, and also in the original discussion that
> led to the introduction of that option.  
>
> While I think the variable losing "index" from its name was a vast
> improvement relative to "--no-lock-index", simply because it
> expresses what we do a bit closer to "do not just do things without
> modifying anything my repository", it did not go far enough.

Yuck, the last sentence was garbled.  What I meant as the ideal
"read-only" was "do things without modifying anything in my
repository".

And to avoid any misunderstanding, what I mean by "it did not go far
enough" is *NOT* this:

We added a narrow feature and gave it a narrow name.  Instead we
should have added a "--read-only" feature, which this change may
be a small part of, and waited releasing the whole thing until
it is reasonably complete.

By going far enough, I was wondering if we should have done
something that we historically did not do.  An "aspirational"
feature that is incrementally released with a known bug and that
will give users what they want in the larger picture when completed.

IOW, we could have made this "git --read-only ", that is
explained initially as "tell Git not to modify repository when it
does not have to (e.g. avoid opportunistic update)" and perhaps
later as "tell Git not to modify anything in the repository--if it
absolutely has to (e.g. "git --read-only commit" is impossible to
complete without modifying anything in the repository), error out
instead".  And with a known-bug section to clearly state that this
feature is not something we vetted every codepath to ensure the
read-only operation, but is still a work in progress.

After all, "status" does not have to stay to be the only command
with opportunistic modification (in the current implementation, it
does "update-index --refresh" to update the index).  And the index
does not have to stay to be the only thing that is opportunistically
modified (e.g. "git diff --cached" could not just opportunistically
update the index, but also it could be taught to write out tree
objects for intermediate directories when it does its cache-tree
refresh, which would help the diff-index algorithm quite a bit in
the performance department).  

Having a large picture option like "--read-only" instead of ending
up with dozens of "we implemented a knob to tweak only this little
piece, and here is an option to trigger it" would help users in the
long run, but we traditionally did not do so because we tend to
avoid shipping "incomplete" features, but being perfectionist with
such a large undertaking can stall topics with feature bloat.  In a
case like this, however, I suspect that an aspirational feature that
starts small, promises little and can be extended over time may be a
good way to move forward.

Re: Clone repository computer A to remote B doenst work

[Roberto Garcia]

Thanks you very much:
I have QNAP NAS,
Finally I have installed QGit (from forum QNAP because you can't find
in app center) from here:
https://forum.qnap.com/viewtopic.php?f=320=109649
I used SSH for create a bare repository in the server side.
Once it was created i went to the local machine and i wrote:
git clone user@ip:/my/path/in/the/server/git
I put my files, add and commit to the new git repository created by git clone.
Now i can use git push to commit the changes into the server

Thanks you very much!!!
Regards,
Roberto


2017-11-25 23:38 GMT+01:00 Johannes Schindelin :
> Hi Roberto,
>
> On Sat, 25 Nov 2017, Roberto Garcia wrote:
>
>> I'm trying clone in windows a git repository to other remote machine
>> (NAS Linux based).
>> I have installed git for windows but i didn't installed nothing in the
>> other remote machine (NAS Linux based).
>
> You need a Git on the remote side. Otherwise Git will not be able to clone
> from there.
>
> Ciao,
> Johannes

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sun, Nov 26, 2017 at 12:46:12PM +0900, Junio C Hamano wrote:
> Max Kirillov  writes:
> > +ssize_t git_env_ssize_t(const char *k, ssize_t val)
> > +{
> > +   const char *v = getenv(k);
> > +   if (v && !git_parse_ssize_t(v, ))
> > +   die("failed to parse %s", k);
> > +   return val;
> > +}
> > +
> 
> If this were passing "v" that is a string the caller obtains from
> any source (and the callee does not care about where it came from),
> that would be a different story, but as a public interface that is
> part of the config.c layer, "k" that has to be the name of the
> environment variable sticks out like a sore thumb.
> 
> If we were to add one more public function to the interface, I
> suspect that exposing the existing git_parse_ssize_t() and have the
> caller implement the above function for its use would be a much
> better way to go.

I'm afraid I did not get the reasonsing and not fully the
desired change. Is this http-backend code change (compared
to the last patch) what you mean?

--- a/http-backend.c
+++ b/http-backend.c
@@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t 
req_len, unsigned char **o
}
 }
 
+static ssize_t env_content_length()
+{
+   const char *str = getenv("CONTENT_LENGTH");
+   ssize_t val = -1;
+   if (str && !git_parse_ssize_t(str, ))
+   die("failed to parse CONTENT_LENGTH: %s", str);
+   return val;
+}
+
 static ssize_t read_request(int fd, unsigned char **out)
 {
-   ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+   ssize_t req_len = env_content_length();
if (req_len < 0)
return read_request_eof(fd, out);
else