Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Eric Sunshine]

On Mon, Nov 27, 2017 at 11:04 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> I had envisioned a simple 'goto remove_control_info' rather than extra
>> nesting or refactoring, but that's a minor point.
>
> Yes, use of goto is also a good way to avoid having to worry about
> the future evolution of the codeflow in this function.
>
> So perhaps
>
> if (the directory is no longer there)
> goto cleanup;
> if (the worktree does not validate)
> return -1;
> ... the original code to (carefully) remove the
> ... worktree itself
>
> cleanup:
>
> ... remove control info
> ... free resources held in variables
> ... return from the function
>
> is what we want?

Yes, that's what I had in mind.

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

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> without having "wish" on the build machine. If everybody is happy with
>> the BYPASS mechanism you added to address that, then I'm perfectly fine
>> with it.
>
> OK.  The topic was queued on 'pu' yesterday; lets move it forward
> after waiting for a few more days to see if there are further
> improvements and/or objections.

It seems that TravisCI objects ;-)

https://travis-ci.org/git/git/jobs/307745929

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

Jeff King  writes:

> ... "if we need advice.*, that is a good sign that the
> feature is mis-designed".
>
> Let me elaborate a bit on the latter.
>
> My gut feeling is that this is absolutely the wrong place to put a
> message like this
> The right place for this message, IMHO, is for the editor itself (or a
> wrapper script) to say "hey, I'm opening a new window" (like emacsclient
> does).

Yes, I think we already had that discussion and if you recall those
involved in the thread except Lars were in favor of writing this off
as "here is a nickel--get a better editor".

One thing that may sway you slightly in favor of doing this in Git
is that a new user _might_ not be expecting Git to open an editor in
response to the command s/he just gave it [*1*].

It is one thing that the user _knowingly_ runs an editor and fails
to locate which window the editor opened.  It is a bit different if
the user didn't even know that s/he is supposed to be interacting
with an editor.

> But I also recognize that the world isn't perfect. Not all editors will
> get this right, and not all users are savvy enough to set up a wrapper
> script for editors which don't. So defaulting this to "on" tries to help
> those cases.
>
> If the anti-cruft techniques I mentioned above work well in practice,
> then we get to have our cake and eat it, too. If they don't, then I'm
> not sure if the tradeoff is worth it.

I agree with all of the above; I do not know if the "go back and
erase to the end of the line" is good enough in practice, and that
is why I sent "here is a possible anti-cruft" but did not push it
strongly forward myself.


[Footnote]

*1* "git merge topic" used to just complete the merge with canned
log message and people were not all that unhappy--they just
thought it is an OK design that a clean merge is a
non-interactive operation.  Perhaps a person new to Git may be
expecting that behaviour (without knowing anything like (1) that
was what we used to do or (2) we open an editor by default these
days), wondering why nothing seems to be happening, staring at
an inactive window, after typing "git merge topic".

Re: [PATCH] repository: fix a sparse 'using integer as NULL pointer' warning

[Junio C Hamano]

Ramsay Jones  writes:

> Hi Junio,
>
> I don't recall Brian doing a re-roll of the 'bc/hash-algo' branch[1], but
> now that it has been merged into the 'next' branch, sparse is barking on
> that branch too. This patch (slightly different to the last one) applies
> on top of 'next'.

Thanks.  Will queue on that topic.

Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Junio C Hamano]

Eric Sunshine  writes:

> With this approach, validate_worktree() will print an error message
> saying that the worktree directory is missing before the control info
> is actually removed. Kaartic's original patch silenced the message
> (and _all_ error messages from validate_worktree()) by passing 1 as
> the second argument. That seemed heavy-handed, so I suggested keeping
> the validate_worktree() call as-is but doing the directory-missing
> check first as a special case.
>
> But perhaps that special case isn't necessary.

I do not think the user minds to see "there is no such directory
there"; actually that would be beneficial, even.

But you are right; validate_worktree() would need to become aware of
the possibility that it can be passed such a "corrupt" worktree to
handle if that approach is followed.

The case we are discussing, i.e. the user removed the directory
without telling Git to give it a chance to remove control
information, may be common enough that it becomes a worthwhile
addition to make the "quiet" boolean that occupies a whole int to an
unsigned that is a collection of bits, and pass "missing_ok" bit in
that flag word to the validate_worktree() to tell that the caller
can tolerate the "user removed it while we were looking the other
way" case and can handle it gracefully.  But that would be a lot
larger change, and "the caller checks before calling validate" as
done with this [RFC v2] may be a good way to add the feature with
the least impact to the codebase.

> I had envisioned a simple 'goto remove_control_info' rather than extra
> nesting or refactoring, but that's a minor point.

Yes, use of goto is also a good way to avoid having to worry about
the future evolution of the codeflow in this function.

So perhaps

if (the directory is no longer there)
goto cleanup;
if (the worktree does not validate)
return -1;
... the original code to (carefully) remove the 
... worktree itself

cleanup:

... remove control info
... free resources held in variables
... return from the function

is what we want?

In any case, I think this needs thawing nd/worktree-move topic
before it can move forward, as all the functions we discussed in
this thread are the ones that were introduced in that topic and do
not exist in the mainline.

Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Eric Sunshine]

On Mon, Nov 27, 2017 at 10:02 PM, Junio C Hamano  wrote:
> I actually wonder if this "early check and return" is making the
> code unmaintainable.  What if we instead did it with just the
> codeflow restructuring, perhaps like so?
>
> if (!validate_worktree(wt, 0)) {
> /* OK, work tree is sound */
> if (!force) {
> /* protect from information lossage */
> }
> /* do the actual worktree removal */
> }
> /* remove the control info */

With this approach, validate_worktree() will print an error message
saying that the worktree directory is missing before the control info
is actually removed. Kaartic's original patch silenced the message
(and _all_ error messages from validate_worktree()) by passing 1 as
the second argument. That seemed heavy-handed, so I suggested keeping
the validate_worktree() call as-is but doing the directory-missing
check first as a special case.

But perhaps that special case isn't necessary. Perhaps I was
overcautious about having those error messages silenced. After all,
the user has explicitly asked to delete the worktree, so (presumably)
nothing is lost by honoring that request, even if there are validation
errors. (We could even add a --verbose option to 'remove' to re-enable
those messages, but that's probably overkill.)

> There is no need for a new helper function when done that way, which
> allows us not to worry about two clean-up places drifting apart over
> time.

I had envisioned a simple 'goto remove_control_info' rather than extra
nesting or refactoring, but that's a minor point.

Re: [PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Junio C Hamano]

Dan Jacques  writes:

>> In Git for Windows, we have an almost identical patch:
>>
>> https://github.com/git-for-windows/git/commit/bdd739bb2b0b
>>
>> We just guard the call to system_path() behind a test whether podir is
>> already absolute, but these days, system_path() does that itself.
>>
>> I am too little of a Perl expert to be helpful with the other patches, but
>> I would gladly runa build & test on Windows if you direct me to an
>> easily-pullable branch.
>
> Oh interesting - I've only peripherally looked at Git-for-Windows code,
> since Chromium uses its packages verbatim (thanks, BTW!). I think you're
> correct though - this patch set seems to be doing the same thing.
>
> I've been force-pushing my changes to the "runtime-prefix" branch of my Git
> fork for travis.ci testing. The latest commit on that branch adds a
> "config.mak" for testing, so one commit from the branch head will contain
> the sum set of this patch series applied at (or near) Git's master branch:
>
> https://github.com/danjacques/git/tree/runtime-prefix~1
>
> Let me know if this is what you are looking for, and if I can offer any
> help with Windows testing. Thanks!

FWIW, I plan to include this somewhere on 'pu' for today's
integration cycle, so dj/runtime-prefix topic branch would also be
what can easily be grabbed.

Re: [PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Dan Jacques]

> In Git for Windows, we have an almost identical patch:
>
> https://github.com/git-for-windows/git/commit/bdd739bb2b0b
>
> We just guard the call to system_path() behind a test whether podir is
> already absolute, but these days, system_path() does that itself.
>
> I am too little of a Perl expert to be helpful with the other patches, but
> I would gladly runa build & test on Windows if you direct me to an
> easily-pullable branch.

Oh interesting - I've only peripherally looked at Git-for-Windows code,
since Chromium uses its packages verbatim (thanks, BTW!). I think you're
correct though - this patch set seems to be doing the same thing.

I've been force-pushing my changes to the "runtime-prefix" branch of my Git
fork for travis.ci testing. The latest commit on that branch adds a
"config.mak" for testing, so one commit from the branch head will contain
the sum set of this patch series applied at (or near) Git's master branch:

https://github.com/danjacques/git/tree/runtime-prefix~1

Let me know if this is what you are looking for, and if I can offer any
help with Windows testing. Thanks!

Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Junio C Hamano]

Kaartic Sivaraam  writes:

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index b5afba164..6eab91889 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const 
> char *prefix)
>   return update_worktree_location(wt, dst.buf);
>  }
>  
> +/* Removes the .git/worktrees/worktree_id directory for
> + * the given worktree_id
> + *
> + * Returns 0 on success and non-zero value in case of failure
> + */

/*
 * our multi-line comment should be formatted
 * more like this, giving slash-asterisk at the
 * beginning and asterisk-slash at the end their
 * own line.
 */

There are other instances of the same in this patch, I suspect, but
because this seemed to depend on other things in 'pu' that are not
ready (if it depends on something that is stalled or abandoned, we
need to first get it unabandoned before this can even come close to
'pu'), I didn't create a topic branch for this RFC patch to view the
resulting file as a whole (this review is based only on the patch
e-mail).

> +static int remove_worktree_entry(char *worktree_id) {
> + int ret = 0;
> + struct strbuf we_path = STRBUF_INIT;
> + strbuf_addstr(_path, git_common_path("worktrees/%s", worktree_id));
> + if (remove_dir_recursively(_path, 0)) {
> + error_errno(_("failed to delete '%s'"), we_path.buf);
> + ret = -1;
> + }
> + strbuf_release(_path);
> + return ret;
> +}
> +

This lifts a small section of remove_worktree() to make it usable
from other places.  But see below.

>  static int remove_worktree(int ac, const char **av, const char *prefix)
>  {
>   int force = 0;
> @@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, 
> const char *prefix)
>   die(_("already locked, reason: %s"), reason);
>   die(_("already locked, no reason"));
>   }
> +
> + if (!file_exists(wt->path)) {
> + /* There's a worktree entry but the worktree directory
> +  * doesn't exist. So, just remove the worktree entry.
> +  */
> + ret = remove_worktree_entry(wt->id);
> + free_worktrees(worktrees);
> + return ret;
> + }
> +
>   if (validate_worktree(wt, 0))
>   return -1;

I actually wonder if this "early check and return" is making the
code unmaintainable.  What if we instead did it with just the
codeflow restructuring, perhaps like so?

if (!validate_worktree(wt, 0)) {
/* OK, work tree is sound */
if (!force) {
/* protect from information lossage */
}
/* do the actual worktree removal */
}
/* remove the control info */

There is no need for a new helper function when done that way, which
allows us not to worry about two clean-up places drifting apart over
time.  With this patch, we have two 3-line blocks that call
remove_worktree_entry(wt->id), free_worktrees(worktrees) and returns
ret, and these happen to be identical, but the next person who would
be mucking with the code (perhaps adding more variables that need to
be reset in this codeflow) can easily miss one of these two places.

The resulting code would make the body of "if (!force)" block too
deeply nested, I suspect, but that is an indication that that part
is overlong and overly complex in the context of this function, and
can and should migrate to its own helper function.

Hmm?

> @@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, 
> const char *prefix)
>   error_errno(_("failed to delete '%s'"), sb.buf);
>   ret = -1;
>   }
> - strbuf_reset();
> - strbuf_addstr(, git_common_path("worktrees/%s", wt->id));
> - if (remove_dir_recursively(, 0)) {
> - error_errno(_("failed to delete '%s'"), sb.buf);
> - ret = -1;
> - }
> - strbuf_release();
> + ret = remove_worktree_entry(wt->id);
>   free_worktrees(worktrees);
>   return ret;
>  }

[PATCH] repository: fix a sparse 'using integer as NULL pointer' warning

[Ramsay Jones]

Commit 78a6766802 ("Integrate hash algorithm support with repo setup",
2017-11-12) added a 'const struct git_hash_algo *hash_algo' field to the
repository structure, without modifying the initializer of the 'the_repo'
variable. This does not actually introduce a bug, since the '0' initializer
for the 'ignore_env:1' bit-field is interpreted as a NULL pointer (hence
the warning), and the final field (now with no initializer) receives a
default '0'.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

I don't recall Brian doing a re-roll of the 'bc/hash-algo' branch[1], but
now that it has been merged into the 'next' branch, sparse is barking on
that branch too. This patch (slightly different to the last one) applies
on top of 'next'.

ATB,
Ramsay Jones

[1] 
https://public-inbox.org/git/%3c91150cfc-3271-16b0-33d3-9a4e149dc...@ramsayjones.plus.com%3E/

 repository.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repository.c b/repository.c
index c6ceb9f9e..998413b8b 100644
--- a/repository.c
+++ b/repository.c
@@ -5,7 +5,7 @@
 
 /* The main repository */
 static struct repository the_repo = {
-   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 0, 0
+   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, NULL, 
0, 0
 };
 struct repository *the_repository = _repo;
 
-- 
2.15.0

Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Junio C Hamano]

Kaartic Sivaraam  writes:

> When the N-th previous thing checked out sytax is used with
> '--branch' option of check-ref-format the results might not
> always be a valid branch name

I wonder if you want to rephrase this, because 40-hex object name is
syntactically a valid branch name.  It's (1) cumbersome to type and
(2) may not be what the user expects.

I have a mild suspicion that "git checkout -B @{-1}" would want to
error out instead of creating a valid new branch whose name is
40-hex that happen to be the name of the commit object you were
detached at previously.

I am not sure if "check-ref-format --branch" should the same; it is
more about the syntax and the 40-hex _is_ valid there, so...

Re: [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state

[Junio C Hamano]

Kaartic Sivaraam  writes:

>   any branch (see below for details).
>  +
> -As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
> -checks out branches (instead of detaching).  You may also specify
> -`-` which is synonymous with `"@{-1}"`.
> +You can use the `"@{-N}"` syntax to refer to the N-th last
> +branch/commit checked out using "git checkout" operation. You may
> +also specify `-` which is synonymous to `"@{-1}`.
>  +
> -As a further special case, you may use `"A...B"` as a shortcut for the
> +As a special case, you may use `"A...B"` as a shortcut for the
>  merge base of `A` and `B` if there is exactly one merge base. You can
>  leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.

Looks sensible.  Will queue.

Thanks.

Re: [PATCH v2 3/3] rebase: rebasing can also be done when HEAD is detached

[Junio C Hamano]

Kaartic Sivaraam  writes:

> + if test "$branch_or_commit" = "HEAD" &&
> +  !(git symbolic-ref -q HEAD)

Did you need a subshell here?  Now with a proper test with
"symbolic-ref -q HEAD", I wonder if you'd need to check if the
original was named HEAD in the first place (I do not feel strongly
enough to say that checking is wrong, at least not yet, but the
above does make me wonder), and instead something like

if ! git symbolic-ref -q HEAD
then
...

might be sufficient.  I dunno.

I find what 2/3 and 3/3 want to do quite sensible.  They depend on
1/3, which I find is more a needless churn than a clean-up, which is
unfortunate, though.

Thanks.

Re: [PATCH v2 0/3] rebase: give precise error message

[Junio C Hamano]

Kaartic Sivaraam  writes:

> 1. "git rebase  " does nothing

Not limited to "rebase", you do not muck with remote-tracking branch
in your local repository, so it would be a bug if the above updated
where the remote-tracking branch points at.

The form of "git rebase" with one extra argument (i.e. not rebasing
the history that leads to the current checkout) is mere shorthand of
checking that extra thing out before doing the rebase, i.e.

$ git rebase origin/next origin/maint

first checks out origin/maint (you'd get on a detached HEAD) and
rebase the history leading to the detached HEAD on top of
origin/next.  If it fast-forwards (and it should if you are talking
about 'maint' and 'next' I publish), you'll end up sitting on a
detached HEAD that points at origin/next.

There is nothing to see here.

> 2. It's possible to do "git rebase  "

Again, that's designed behaviour you can trigger by giving 
(not ).  Very handy when you do not trust your upstream or
yourself's ability to resolve potential conflicts as a trial run
before really committing to perform the rebase, e.g.

$ git rebase origin master^0

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

[Junio C Hamano]

mwnx  writes:

> On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:
>> 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.
>
> (especially since I'm not quite sure what the rationale behind it
> is).

As DCO is something we'd need to present to the court when the next
SCO comes after us, we'd prefer to see that we can refer to, and
contact if necessary, a real person when we need to say "this is not
a stolen code, here is the person who presented it to us and we'll
let him or her explain".

> What are your thoughts on this issue?

Not limited to this, but our stance for things are that previous
mistakes do not justify repeating and spreading the same.

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

[Takuto Ikuta]

Hi Johannes,

As long as this PR is included in next Git for Windows release, I
won't suffer from slow git fetch.
https://github.com/git-for-windows/git/pull/1372

But I sent you 2 PRs to follow right way.
https://github.com/git-for-windows/git/pull/1379
https://github.com/git-for-windows/git/pull/1380
Feel free to merge these PRs.

Thanks.

2017-11-28 9:33 GMT+09:00 Johannes Schindelin :
> Hi Junio,
>
> On Tue, 28 Nov 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>>
>> > My current plan is to release a new Git for Windows version on Wednesday,
>> > to allow for a new cURL version to be bundled.
>>
>> Is this an updated 2.15.0 or are you planning to package 2.15.1?
>
> If there is a 2.15.1 to pick up for me, I'll take it. Otherwise it'll be
> Git for Windows v2.15.0(2).
>
> Ciao,
> Dscho



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

Re: [PATCH v2] diff: support anchoring line(s)

[Junio C Hamano]

Jonathan Tan  writes:

> - else if (!strcmp(arg, "--patience"))
> + else if (!strcmp(arg, "--patience")) {
> + int i;
>   options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
> - else if (!strcmp(arg, "--histogram"))
> + /*
> +  * Both --patience and --anchored use PATIENCE_DIFF
> +  * internally, so remove any anchors previously
> +  * specified.
> +  */
> + for (i = 0; i < options->anchors_nr; i++)
> + free(options->anchors[i]);
> + options->anchors_nr = 0;

This makes sense, but "--diff-algorithm=patience" would want to do
the same, I suspect, so the loop would want to become a little
helper function "clear_patience_anchors(options)" or something like
that.

> diff --git a/t/t4064-diff-anchored.sh b/t/t4064-diff-anchored.sh
> new file mode 100755
> index 0..b3f510f04
> --- /dev/null
> +++ b/t/t4064-diff-anchored.sh
> @@ -0,0 +1,94 @@
> +#!/bin/sh
> +
> +test_description='anchored diff algorithm'
> +
> +. ./test-lib.sh
> +
> +test_expect_success '--anchored' '
> + printf "a\nb\nc\n" >pre &&
> + printf "c\na\nb\n" >post &&

This may be too little to matter, but I'd find

printf "%s\n" a b c >pre

vastly easier to read.  Or perhaps just use

test_write_lines a b c >pre

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

[Igor Djordjevic]

Hi Johannes,

On 27/11/2017 22:54, Johannes Sixt wrote:
> 
> I my opinion, putting the focus on integration merge commits and the
> desire to automate the re-merge step brings in a LOT of complexity in
> the implementation for a very specific use-case that does not
> necessarily help other cases.

It might seem more complex than it is, until you examine the guts to 
see how it really works :)

Basic concept is pretty simple, as I actually don`t automate 
anything, at least not in terms of what would manual user steps look 
like - for example, there`s no real re-merge step in terms of 
actually redoing the merge, I just reuse what was already there in a 
very clean way, I would think (supported by my current, humble 
knowledge, still).

The only merge that could possibly ever happen is upon committing 
desired subset of changes onto parent, and that shouldn`t be too 
complex by definition, otherwise that commit doesn`t really belong 
there in the first place, if it can`t be meaningfully applied where 
we want it (for now, at least).

That said, the whole operation of "posting on parent and re-merging 
everything", the way it looks like from the outside, could end just 
with a simple diff-apply-commit-commit internally, no merges at all. 
Only if simple `git apply` fails, we try some trivial merging - and 
all that inside separate (parent) index only, not touching original 
HEAD index nor working tree, staying pristine for the whole process, 
untouched.

Once done, you should be in the very same situation you started from, 
nothing changed, just having your history tweaked a bit to tell a 
different story on how you got there (now including a commit you 
posted on your HEAD`s parent).

Otherwise, I agree that explained use case might be a bit specific, 
but that is only because I recognized that one to be the most 
interesting to initially present (not to say one of more complex 
cases) - to me, at least, but it is certainly not the only one.

Don`t let "usual/preferred/recommended" Git workflow distract you too 
much - one of the reasons I made this is because it also allows _kind 
of_ "vanilla Git" patch queue, where you can quickly work on top of 
the merge head, pushing commits onto parents below, being tips of 
your "queues", putting you up to speed without a need to ever switch 
a branch (hypothetically), until satisfied with what you have, where 
you can slow down and polish each branch separately, as usual.

Like working on multiple branches at the same time, in the manner 
similar to what `git add --patch` allows in regards to working on 
multiple commits at the same time. This just takes it on yet another 
level... hopefully :)

> For example, in my daily work, I have encountered situations where,
> while working on one topic, I made a hot-fix for a different topic.
> There is no demand for a merge step in this scenario.
> 
> In your scenario above, it would certainly not be too bad if you
> forgo the automatic merge and have the user issue a merge command
> manually. The resulting history could look like this:
> 
> (3) o---o---A---X(topicA)
>/ \   \
>   /   M1--M2 (test, HEAD)
>  /   /||
>  ---o---o---M---' || (master)
>  \   \   / |
>   \   o-B /  (topicB)
>\ /
> o---o---C(topicC)
> 
> I.e., commit --onto-parent A produced commit X, but M2 was then a
> regular manual merge. (Of course, I am assuming that the merge
> commits are dispensible, and only the resulting tree is of
> interest.)

I see - and what you`re asking for is what I already envisioned and 
hoped to get some more feedback about, here`s excerpt from 
[SCRIPT/RFC 3/3] git-commit--onto-parent.sh[1] (I guess you didn`t 
have time to checked that one yet?):

  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.

So I definitely look forward decoupling these two ((1) commit to 
parent and (2) remerge), with enough discussion flowing :)

Heck, even "to parent" is an artificial/imposed restriction now, in 
reality you could commit on top of any other commit you want (without 
switching branches)... but let`s take one step at a time.

Just note that omitting the remerge step is what actually makes the 
logic more complex, as we 

Re: [PATCH v3] git-send-email: honor $PATH for sendmail binary

[Junio C Hamano]

Florian Klink  writes:

> This extends git-send-email to also consider sendmail binaries in $PATH
> after checking the (fixed) list of /usr/sbin and /usr/lib, and before
> falling back to localhost.
>
> Signed-off-by: Florian Klink 
> ---

Thanks for an update.

In an ideal world where we were introducing git-send-email for the
first time without any existing users, we would certainly prefer
things on directories listed in $PATH, and use the two traditional
hardcoded places merely as fallback, but because we do have existing
users who have been relying on the code finding /usr/lib/sendmail
(even when they have something called 'sendmail' that they do not
want to use on their $PATH) and doing that ideal implementation
would break things for them.  Those who have /usr/lib/sendmail
installed that they do not want to use can continue to use
sendemail.smtpserver---if $PATH were searched first, they could
instead list the path that has their faviourite sendmail on it
without setting the configuration, but it does not change the fact
that they need to do _something_ anyway, so it is not too huge a
deal.

>  Documentation/git-send-email.txt | 6 +++---
>  git-send-email.perl  | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt 
> b/Documentation/git-send-email.txt
> index bac9014ac..44db25567 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -203,9 +203,9 @@ a password is obtained using 'git-credential'.
>   specify a full pathname of a sendmail-like program instead;
>   the program must support the `-i` option.  Default value can
>   be specified by the `sendemail.smtpServer` configuration
> - option; the built-in default is `/usr/sbin/sendmail` or
> - `/usr/lib/sendmail` if such program is available, or
> - `localhost` otherwise.
> + option; the built-in default is to search for `sendmail` in
> + `/usr/sbin`, `/usr/lib/sendmail` and $PATH if such program is
> + available, falling back to `localhost` otherwise.

"search for `sendmail` in `/usr/sbin`, `/usr/lib/sendmail`" would
mean we would not be happy with /usr/lib/sendmail but would be with
either /usr/sbin/sendmail or /usr/lib/sendmail/sendmail, which is
not what you wanted to say.  I'd do 's|/usr/lib/sendmail|/usr/lib|'
while queueing.

Thanks again.

>  --smtp-server-port=::
>   Specifies a port different from the default port (SMTP
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..edcc6d346 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -885,7 +885,9 @@ if (defined $initial_reply_to) {
>  }
>  
>  if (!defined $smtp_server) {
> - foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
> + my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
> + push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
> + foreach (@sendmail_paths) {
>   if (-x $_) {
>   $smtp_server = $_;
>   last;

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

[Elijah Newren]

On Mon, Nov 27, 2017 at 3:39 PM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>>> As a fix, this sorely wants something new in t/ directory.
>>
>> Well, then perhaps I was wrong to submit it independent of my
>> directory rename series.  As noted in the (very lengthy) extended
>> commit message explanation, the assumption the previous code made just
>> happened to work ...
>
> Here is what I wrote in What's cooking draft (which automatically
> gets copied to the merge log message and becomes part of release
> notes when a topic graduates) for this thing.  Am I on the right
> track?
>
> The code internal to the recursive merge strategy was not fully
> prepared to see a path that is renamed to try overwriting another
> path that is only different in case on case insensitive systems.
> This does not matter in the current code, but will start to matter
> once the rename detection logic starts taking hints from nearby
> paths moving to some directory and moves a path that is otherwise
> not changed along with them.

Yes, though I have one minor nit: I'd prefer "a new path" to "a path
that is otherwise not changed".

(Reason for the nit: "Not changed" to me implies the file existed in
the merge base and didn't change name on either side of the merge,
which in turn implies the directory remains on both sides, which means
there was no directory rename.  Since directory rename detection is
mostly about moving file adds into the right directory, this seemed
like the simplest correction.  There are also transitive renames, but
they're much less common and trying to cover them too might make it
even harder to parse.)

As a side note, the two sentences are a little bit unwieldy to try to
parse.  I couldn't come up with any concrete suggestions to improve
it, so it's probably fine, but thought I'd mention in case others spot
an easy way to simplify it.

[PATCH v3] git-send-email: honor $PATH for sendmail binary

[Florian Klink]

This extends git-send-email to also consider sendmail binaries in $PATH
after checking the (fixed) list of /usr/sbin and /usr/lib, and before
falling back to localhost.

Signed-off-by: Florian Klink 
---
 Documentation/git-send-email.txt | 6 +++---
 git-send-email.perl  | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index bac9014ac..44db25567 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -203,9 +203,9 @@ a password is obtained using 'git-credential'.
specify a full pathname of a sendmail-like program instead;
the program must support the `-i` option.  Default value can
be specified by the `sendemail.smtpServer` configuration
-   option; the built-in default is `/usr/sbin/sendmail` or
-   `/usr/lib/sendmail` if such program is available, or
-   `localhost` otherwise.
+   option; the built-in default is to search for `sendmail` in
+   `/usr/sbin`, `/usr/lib/sendmail` and $PATH if such program is
+   available, falling back to `localhost` otherwise.
 
 --smtp-server-port=::
Specifies a port different from the default port (SMTP
diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..edcc6d346 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -885,7 +885,9 @@ if (defined $initial_reply_to) {
 }
 
 if (!defined $smtp_server) {
-   foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
+   my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
+   push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
+   foreach (@sendmail_paths) {
if (-x $_) {
$smtp_server = $_;
last;
-- 
2.15.0

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

[Johannes Schindelin]

Hi Junio,

On Tue, 28 Nov 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > My current plan is to release a new Git for Windows version on Wednesday,
> > to allow for a new cURL version to be bundled.
> 
> Is this an updated 2.15.0 or are you planning to package 2.15.1?

If there is a 2.15.1 to pick up for me, I'll take it. Otherwise it'll be
Git for Windows v2.15.0(2).

Ciao,
Dscho

Re: [PATCH] git-send-email: honor $PATH

[Florian Klink]

Support to detect sendmail binaries in windows' PATH seems a bit more complex.
The separator is different, and PATHEXT would need to be considered too.  I'm
not even sure if having a sendmail binary in PATH on windows is something usual
or if defaulting to smtp to localhost (what we currently do) is good enough 
(tm).
If we want to start parsing PATH under windows too, I'd suggest to use
File::Which instead of implementing it on our own.


I would feel a lot more worried about trying elements on the $PATH
first and then using the two standard places as fallback.  If the
order of addition matters at all, that would mean that trying
elements on $PATH first and then falling back to the two standard
places *will* change the behaviour---for the affected users, we used
to pick one of these two, but now we would pick something different.
sendmail is usually installed out of the way of $PATH for regular
users for a reason, so picking anything whose name happens to be
sendmail that is on $PATH does not sound right.

Of course, for users who do not have sendmail at one of the two
standard places _and_ has one on one of the directories on $PATH,
the order in which we check would not make a difference, so my
suggestion would be to do the other way around.


I could happily provide a patch that does it the other way round, too. But let's
first decide on what to do with windows ;-)


Seems like there is not really much of motivation to try better in detecting
sendmail binaries in PATH on windows ;-)

Will send patch v3, which reverses the order as suggested by Junio shortly.

--
Florian

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

[Junio C Hamano]

Johannes Schindelin  writes:

> My current plan is to release a new Git for Windows version on Wednesday,
> to allow for a new cURL version to be bundled.

Is this an updated 2.15.0 or are you planning to package 2.15.1?  I
am asking because you earlier asked me to hold 2.15.1 while you were
away last week and there are accumulated changes on 'maint'.

As I won't be online later this week, unless I cut 2.15.1 today or
early tomorrow, it will have to be done early next week, and I'd
prefer not to do 2.15.1 _immediately_ after a platform announces
an updated release of 2.15.0 for obvious reasons.

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

[Junio C Hamano]

Johannes Schindelin  writes:

> could I ask for a favor? I'd like the oneline to start with
>
>   rebase -i -x: ...
>
> (this would help future me to realize what this commit touches already
> from the concise graph output I favor).

Excellent.

>> 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.
>
> Since Junio did not comment on the commit message: could you replace
> `aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

Yes, I noticed them but don't mind me ;-)  The above are all good fixes.

All suggestions in the remainder looked sensible.  Thanks for a
review.

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> 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.
>
> How about a "yes, and" instead? As in:
>
> To further improve this patch, let's use the name
> sequencer_add_exec_commands() for this function because it is defined
> globally now.

I would do so when I have a single "this is strictly better"
suggestion.  In this case, I didn't, but somebody who does not have
a "better suggestion" can still have a good sense of smell to tell
something is "not right".

>> > +  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.
>
> Funny, I would have assumed it the other way round: since "first" always
> has to be initialized with 1, I would have been surprised to see an
> explicit assignment much later than it is declared.

Unfortunately that would force readers to see what happens before
the loop to see if there are cases where first is incremented, and
in this case there is not any.

Re: [PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Johannes Schindelin]

Hi Dan,

On Mon, 27 Nov 2017, Dan Jacques wrote:

> diff --git a/gettext.c b/gettext.c
> index db727ea02..6b64d5c2e 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -2,7 +2,8 @@
>   * Copyright (c) 2010 Ævar Arnfjörð Bjarmason
>   */
>  
> -#include "git-compat-util.h"
> +#include "cache.h"
> +#include "exec_cmd.h"
>  #include "gettext.h"
>  #include "strbuf.h"
>  #include "utf8.h"
> @@ -157,10 +158,11 @@ static void init_gettext_charset(const char *domain)
>  
>  void git_setup_gettext(void)
>  {
> - const char *podir = getenv("GIT_TEXTDOMAINDIR");
> + const char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);
>  
>   if (!podir)
> - podir = GIT_LOCALE_PATH;
> + podir = system_path(GIT_LOCALE_PATH);

In Git for Windows, we have an almost identical patch:

https://github.com/git-for-windows/git/commit/bdd739bb2b0b

We just guard the call to system_path() behind a test whether podir is
already absolute, but these days, system_path() does that itself.

I am too little of a Perl expert to be helpful with the other patches, but
I would gladly runa build & test on Windows if you direct me to an
easily-pullable branch.

Ciao,
Johannes

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

[Junio C Hamano]

Jeff King  writes:

> without having "wish" on the build machine. If everybody is happy with
> the BYPASS mechanism you added to address that, then I'm perfectly fine
> with it.

OK.  The topic was queued on 'pu' yesterday; lets move it forward
after waiting for a few more days to see if there are further
improvements and/or objections.

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

[Junio C Hamano]

Elijah Newren  writes:

>> As a fix, this sorely wants something new in t/ directory.
>
> Well, then perhaps I was wrong to submit it independent of my
> directory rename series.  As noted in the (very lengthy) extended
> commit message explanation, the assumption the previous code made just
> happened to work ...

Here is what I wrote in What's cooking draft (which automatically
gets copied to the merge log message and becomes part of release
notes when a topic graduates) for this thing.  Am I on the right
track?

The code internal to the recursive merge strategy was not fully
prepared to see a path that is renamed to try overwriting another
path that is only different in case on case insensitive systems.
This does not matter in the current code, but will start to matter
once the rename detection logic starts taking hints from nearby
paths moving to some directory and moves a path that is otherwise
not changed along with them.

Re: [PATCHv5 7/7] builtin/describe.c: describe a blob

[Junio C Hamano]

Stefan Beller  writes:

>> I was reacting to your "fixed".  So will we see a rerolled series or
>> not?
>
> I was not planning on rerolling it.

OK.  Then I do not have to worry about this one and drop it perhaps.
One less topic on 'pu' is always good, if it is not active.

Enjoy your vacation.

Thanks.

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

[mwnx]

On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:
> 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.

I did read that document, but I saw a few commits which were signed
off with an alias so I figured the rule might be flexible
(especially since I'm not quite sure what the rationale behind it
is). I'd rather not give my full name if I can avoid it, however if
I really can't convince you to accept this patch any other way, I
guess I'll comply. What are your thoughts on this issue?

-- 
mwnx
GPG: AEC9 554B 07BD F60D 75A3  AF6A 44E8 E4D4 0312 C726

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> diff to v2:
> - shortened and localized the "waiting" message
> - detect "emacsclient" and suppress "waiting" message

Thanks for moving this forward.


> + static const char *close_notice = NULL;

Because this thing is "static", the variable is NULL when the first
call to this function is made, and the value left in the variable
when a call returns will be seen by the next call.

> + if (isatty(2) && !close_notice) {

Declaring a "static" variable initialized to NULL and checking its
NULL-ness upfront is a common pattern to make sure that the code
avoids repeated computation of the same thing.  The body of the if
statement is run only when standard error stream is a tty (hinting
an interactive session) *and* close_notice is (still) NULL.

> + char *term = getenv("TERM");
> +
> + if (term && strcmp(term, "dumb"))
> + /*
> +  * go back to the beginning and erase the
> +  * entire line if the terminal is capable
> +  * to do so, to avoid wasting the vertical
> +  * space.
> +  */
> + close_notice = "\r\033[K";
> + else if (term && strstr(term, "emacsclient"))
> + /*
> +  * `emacsclient` (or `emacsclientw` on Windows) 
> already prints
> +  * ("Waiting for Emacs...") if a file is opened 
> for editing.
> +  * Therefore, we don't need to print the editor 
> launch info.
> +  */
> + ;
> + else
> + /* otherwise, complete and waste the line */
> + close_notice = _("done.\n");
> + }

It assigns a non-NULL value to close_notice unless the editor is
emacsclient (modulo the bug that "emacsclient" is to be compared
with EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be
used to pick the right one).  For a user of that particular editor,
it is left as NULL.  Because it is unlikely that EDITOR etc. would
change across calls to this function, for them, and only for them,
the above is computed to yield the same result every time this
function is called.

That feels a bit uneven, doesn't it?

There are two possible solutions:

1. drop "static" from the declaration to stress the fact that the
   variable and !close_notice in the upfront if() statement is not
   avoiding repeated computation of the same thing, or

2. arrange that "emacsclient" case also participates in "avoid
   repeated computation" dance.  While at it, swap the order of
   checking isatty(2) and !close_notice (aka "have we done this
   already?)--because we do not expect us swapping file descriptor
   #2 inside this single process, we'd be repeatedly asking
   isatty(2) for the same answer.

The former may be simpler and easier, as an editor invocation would
not be a performance critical codepath.

If we want to do the latter, a cleaner way may be to have another
static "static int use_notice_checked = 0;" declared, and then

if (!use_notice_checked && isatty(2)) {
... what you currently have, modulo the
... fix for the editor thing, and set
... close_notice to a string (or NULL).
use_notice_checked = 1;
}

The users of close_notice after this part that use !close_notice
as "do not give the notice at all" flag and also as "this is the
string to show after editor comes back" can stay the same if you go
this route.  That would be solution 2a.

Of course, you can instead use close_notice = "" (empty string) as a
signal "we checked and we know that we are not using the notice
thing".  If you go that route, then the users after this if statement
that sets up close_notice upon the first call would say !*close_notice
instead of !close_notice when they try to see if the notice is in use.
That would be solution 2b.

I personally think any one of 1., 2a., or 2b. is fine.

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

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

> 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.

Makes sense. As to the diff:

> 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
> +'

In addition to Junio's suggestion to include the "expected" block in the
next test case, I would be in favor of combining all the new code in a
single test case.

Also, I think that the test_commit calls can be simplified to:

test_commit first &&
test_commit second &&
test_commit "fixup! first" first A dummy1 &&
test_commit "squash! second" second B dummy2 &&

> +cat >expected < +p $(git rev-list --abbrev-commit -1 first) first

Maybe $(git rev-parse --short HEAD~3)?

> +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
> +
> +test_expect_success 'respects rebase.abbreviateCommands with fixup, squash 
> and exec' '
> + test_when_finished "
> + git checkout master &&
> + test_might_fail git branch -D abbrevcmd &&
> + test_might_fail git rebase --abort
> + " &&
> + git checkout abbrevcmd &&
> + set_cat_todo_editor &&
> + test_config rebase.abbreviateCommands true &&
> + test_must_fail git rebase -i --exec "git show HEAD" \
> + --autosquash master >actual &&
> + test_cmp expected actual
> +'

Otherwise, it looks good!

Thank you for staying on the ball and getting this patch series updated.

Ciao,
Dscho

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

[Jeff King]

On Tue, Nov 28, 2017 at 12:04:45AM +0100, Johannes Schindelin wrote:

> > +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
> > +   ...
> > +---
> 
> I *think* that AsciiDoc will render this in a different way from what we
> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
> single + in an otherwise empty line to start a new indented paragraph *and
> then continue with non-indented lines*.

Good catch. Interestingly enough, my asciidoc seems to render this
as desired for the docbook/roff version, but has screwed-up indentation
for the HTML version.

Fixing it as you suggest makes it look good in both (and I think you can
never go wrong with "+"-continuation, aside from making the source a bit
uglier).

Squashable patch below for convenience, since I did try it.

-Peff

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 0820b60f6e..42e1ba7575 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -34,18 +34,19 @@ rebase.instructionFormat::
 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:
-
++
+instead of:
++
 ---
pick deadbee The oneline of the commit
pick fa1afe1 The oneline of the next commit
...
 ---
-   Defaults to false.
++
+Defaults to false.

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Jeff King]

On Mon, Nov 27, 2017 at 08:09:32PM +, brian m. carlson wrote:

> > Show a message in the original terminal and get rid of it when the
> > editor returns.
> [...]
> 
> Sorry for coming to the topic so late, but it occurred to me that we
> might want to conditionalize this on an advice.* flag.  I expect there
> are some people who will never want to see this, and letting them turn
> it off would be good.

I am torn between saying "yes please, I would absolutely set such an
option myself" and "if we need advice.*, that is a good sign that the
feature is mis-designed".

Let me elaborate a bit on the latter.

My gut feeling is that this is absolutely the wrong place to put a
message like this. We don't know enough about what the editor is doing,
so we have to take pains to avoid a crufty message in the terminal,
including:

  - playing ANSI-term trickery to erase the message

  - hard-coding (!) emacsclient as a special case

And that's why I say that "advice.*" is a bad sign, because it means
those other techniques are failing, and somebody is seeing and being
annoyed by the cruft.

The right place for this message, IMHO, is for the editor itself (or a
wrapper script) to say "hey, I'm opening a new window" (like emacsclient
does).

But I also recognize that the world isn't perfect. Not all editors will
get this right, and not all users are savvy enough to set up a wrapper
script for editors which don't. So defaulting this to "on" tries to help
those cases.

If the anti-cruft techniques I mentioned above work well in practice,
then we get to have our cake and eat it, too. If they don't, then I'm
not sure if the tradeoff is worth it.

-Peff

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

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

> 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
> + ...
> +---

I *think* that AsciiDoc will render this in a different way from what we
want, but I am not an AsciiDoc expert. In my hands, I always had to add a
single + in an otherwise empty line to start a new indented paragraph *and
then continue with non-indented lines*.

> 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;

My initial reaction was: should we return comment_line_char instead of -1
here? Only after reading how this is called did I realize that the idea is
to use full command names if there is no abbreviation. Not sure whether
this is worth a code comment. What do you think?

> +}
> +
>  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;
>  

I would prefer this hunk to be skipped, it does not really do anything if
I understand correctly.

> @@ -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 **argv)
> +int sequencer_make_script(int keep_empty, int abbreviate_commands, FILE *out,
> +   int argc, const char **argv)
>  {
>   char *format = NULL;
>   struct pretty_print_context pp = {0};
> @@ -2483,7 +2491,9 @@ int sequencer_make_script(int keep_empty, FILE *out,
>   strbuf_reset();
>   if (!keep_empty && is_original_commit_empty(commit))
>   strbuf_addf(, "%c ", comment_line_char);
> - strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
> + strbuf_addf(, "%s %s ",
> + abbreviate_commands ? "p" : "pick",
> + oid_to_hex(>object.oid));

I guess the compiler will optimize this code so that the conditional is
evaluated only once. Not that this is performance critical ;-)

>   pretty_print_commit(, commit, );
>   strbuf_addch(, '\n');
>   fputs(buf.buf, out);
> @@ -2539,7 +2549,7 @@ int add_exec_commands(const char *command)
>   return 0;
>  }
>  
> -int transform_todo_ids(int shorten_ids)
> +int transform_todo_ids(int shorten_ids, int abbreviate_commands)
>  {
>   const char *todo_file = rebase_path_todo();
>   struct todo_list todo_list = TODO_LIST_INIT;
> @@ -2575,19 +2585,33 @@ int transform_todo_ids(int shorten_ids)
>   todo_list.items[i + 1].offset_in_buf :
>   todo_list.buf.len;
>  
> - if (item->command >= TODO_EXEC && item->command != TODO_DROP)
> - fwrite(p, eol - bol, 1, out);
> - else {
> + if (item->command >= TODO_EXEC && item->command != TODO_DROP) {
> + if (!abbreviate_commands || 
> command_to_char(item->command) < 0) {
> + fwrite(p, eol - bol, 1, out);
> + } else {
> + const char 

Re: git status always modifies index?

[Jeff King]

On Mon, Nov 27, 2017 at 12:57:31PM -0800, Jonathan Nieder wrote:

> > 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 am worried that the project is not learning from what happened here.
> 
> My main issue with the --no-optional-locks name is that it does not
> connect to the underlying user need.  Your main argument for it is
> that it exactly describes the underlying user need.  One of us has to
> be wrong.

Or there's a false dichotomy. ;) We could be talking about two different
users.

> So let me describe my naive reading:
> 
> As a user, I want to inspect the state of the repository without
> disrupting it in any way.  That means not breaking concurrent
> processes and not upsetting permissions.  --read-only seems to
> describe this use case to me perfectly.

That does not match the request that I got from real script writers who
were having a problem. They wanted to avoid lock contention with
background tasks.  They don't care if the repository is modified as long
as it is done in a safe and non-conflicting way.

I agree (as I think I've said already in this thread) that --read-only
would be a superset of that. And that it would probably be OK to have
just gone there in the first place, sacrificing a small amount of
specificity in the name of having fewer knobs for the user to turn.

> If I understood correctly, your objection is that --read-only is not
> specific enough.  What I really want, you might say, is not to break
> concurrent processes.  Any other aspects of being read-only are not
> relevant.  E.g. if I can refresh the on-disk index using O_APPEND
> without disrupting concurrent processes then I should be satisfied
> with that.

Do I have an objection? It's not clear to me that anybody is actually
proposing anything concrete for me to object to.

Are we adding "--read-only"? Are we going back to "status
--no-lock-index"? In either case, are we deprecating
"--no-optional-locks"?

It sounds like you are arguing for the first, and it sounds like Dscho
is arguing for the second. Frankly, I don't really care that much. I've
said all that I can on why I chose the direction I did, and I remain
unconvinced that we have evidence that the current option is somehow
impossible to find. If somebody wants to take us down one of the other
roads, that's fine by me.

> Fair enough, though that feels like overengineering.  But I *still*
> don't see what that has to do with the name "no-optional-locks".  When
> is a lock *optional*?  And how am I supposed to discover this option?

I kind of feel like any answer I give to these questions is just going
to be waved aside. But here are my earnest answers:

  1. You are bit by lock contention, where running operation X ends up
 with some error like "unable to create index.lock: file exists".
 "X" is probably something like "commit".

  2. You search the documentation for options related to locks. You're
 not likely to find it in the manpage for X, since the root of the
 problem actually has nothing to do with X in the first place. It's
 a background task running "status" that is the problem.

  3. You might find it in git(1) while searching for information on
 locks, since "lock" is in the name of the option (and is in fact
 the only hit in that page). The index is also mentioned there
 (though searching for "index" yields a lot more hits).

  4. You might find it in git-status(1) if you suspect that "status" is
 at work. Searching for "index" or "lock" turns up the addition I
 just proposed yesterday.

There are obviously a lot of places where that sequence might fail to
find a hit. But the same is true of just about any option, including
putting "--read-only" into git(1).

> This also came up during review, and I am worried that this review
> feedback is being ignored.  In other words, I have no reason to
> believe it won't happen again.

I'm having a hard time figuring out what you mean here. Do you mean that
I ignored feedback on this topic during the initial review?

Looking at the original thread, I just don't see it. There was some
question about the name. I tried to lay out my thinking here:

  
https://public-inbox.org/git/20170921050835.mrbgx2zryy3ju...@sigill.intra.peff.net/

and ended with:

  I am open to a better name, but I could not come up with one.

There was no meaningful response on the topic. When I reposted v2, I
tried to bring attention to that with:

- there was some discussion over the name. I didn't see other
  suggestions, and I didn't come up with anything better.

So...am I missing something? Am I misunderstanding your point?

> > I would be fine with having a further aspirational "read only" mode.
> 
> Excellent, we seem to agree on this much.  

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

[Johannes Schindelin]

Hi Liam,

could I ask for a favor? I'd like the oneline to start with

rebase -i -x: ...

(this would help future me to realize what this commit touches already
from the concise graph output I favor).

On Sun, 26 Nov 2017, Liam Beguin wrote:

> 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.

Since Junio did not comment on the commit message: could you replace
`aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

> @@ -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),

Maybe `add-exec-commands`? I know it is longer to type, but these options do
not need to be typed interactively and the longer name would be consistent
with the function name.

> 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;
>  }
>  

As the code in add_exec_commands() may appear convoluted (why not simply
append the command after any pick?), the original comment would be really
nice here:

/*
 * Add commands after pick and (series of) squash/fixup commands
 * in the todo list.
 */

> +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);

The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
need to reset it again.

> + 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);

As Junio pointed out so gently: there is a helper function that does this
all very conveniently for us:

if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error_errno(_("could not read '%s'"), todo_file);

And as I realized looking at the surrounding code: you probably just
inherited my inelegant code by copy-editing from another function in
sequencer.c. Should you decide to add a preparatory patch to your patch
series that converts these other callers, or even refactors all that code
that reads the git-rebase-todo file and then parses it, I would be quite
happy... :-) (although I would understand if you deemed this outside the
purpose of your patch series).

> + res = parse_insn_buffer(todo_list.buf.buf, _list);
> + if (res) {
> + todo_list_release(_list);
> + return error(_("unusable todo list: '%s'"), todo_file);
> + }

The variable `res` is not really used here. Let's just put the
parse_insn_buffer() call inside the if ().

> + 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;

This smells like another copy-edited snippet that originated from my
brain, and I am not at all proud by the complexity I used there.

The function should also check for errors during writing. So how about
something like this instead?

struct strbuf *buf = _list.buf;
size_t offset = 0, command_len = strlen(command);
int first = 1, i;
struct todo_item *item;

...

/* insert  before every pick except the first one */
for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++)
if (item->command == TODO_PICK) {
if (first)
first = 0;
else {
strbuf_splice(buf,
  item->offset_in_buf + offset, 0,
  command, command_len);
offset += command_len;
}
}

/* append a 

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

[Johannes Sixt]

Am 26.11.2017 um 23:35 schrieb Igor Djordjevic:

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)


I my opinion, putting the focus on integration merge commits and the 
desire to automate the re-merge step brings in a LOT of complexity in 
the implementation for a very specific use-case that does not 
necessarily help other cases.


For example, in my daily work, I have encountered situations where, 
while working on one topic, I made a hot-fix for a different topic. 
There is no demand for a merge step in this scenario.


In your scenario above, it would certainly not be too bad if you forgo 
the automatic merge and have the user issue a merge command manually. 
The resulting history could look like this:


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

I.e., commit --onto-parent A produced commit X, but M2 was then a 
regular manual merge. (Of course, I am assuming that the merge commits 
are dispensible, and only the resulting tree is of interest.)


Moreover, you seem to assume that an integration branch is an octopus 
merge, that can be re-created easily. I would say that this a very, very 
exceptional situation.




At this point, I spent five minutes thinking of how I would use commit 
--onto-parent if I did not have git-post.


While on the integration branch, I typically make separate commits for 
each fix, mostly because the bugs are discovered and fixed not 
simultaneously, but over time. So, I have a small number of commits that 
I distribute later using my git-post script. But that does not have to 
be so. I think I could work with a git commit --onto-parent feature as 
long as it does not attempt to make a merge commit for me. (I would hate 
that.)


Sometimes, however I have two bug fixes in the worktree, ready to be 
committed. Then the ability to pass pathspec to git commit is useful. 
Does your implementation support this use case (partially staged 
worktree changes)?


Thanks,
-- Hannes

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

[Johannes Schindelin]

Hi Junio,

On Mon, 27 Nov 2017, Junio C Hamano wrote:

> 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.

How about a "yes, and" instead? As in:

To further improve this patch, let's use the name
sequencer_add_exec_commands() for this function because it is defined
globally now.

> > +   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.

Funny, I would have assumed it the other way round: since "first" always
has to be initialized with 1, I would have been surprised to see an
explicit assignment much later than it is declared.

> > +   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?

Ah, this is one of the downsides of patch-based review. If it was reviewed
in context, you would have easily spotted that Liam was merely
copy-editing my code that is still around.

And indeed, I had missed that function when I started to write the
rebase--helper patches.

> > +   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.

While it won't matter in practice, this would be "more correct" to do,
yes.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

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

Makes sense,
Johannes

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

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

>  3 files changed, 34 insertions(+), 48 deletions(-)

Very nice!
Johannes

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

[Johannes Schindelin]

Hi,

On Mon, 27 Nov 2017, Takuto Ikuta wrote:

> 2017-11-27 13:53 GMT+09:00 Junio C Hamano :
> > Jeff King  writes:
> >
> >>> 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.

If you want it in Git for Windows early (where performance is
traditionally worse than on Linux because Git is really optimized for
Linux), I would not mind a Pull Request at
https://github.com/git-for-windows/git.

My current plan is to release a new Git for Windows version on Wednesday,
to allow for a new cURL version to be bundled.

Ciao,
Johannes

Re: Problem installing Git

[Phil Martel]

On 11/25/2017 5:16 PM, Johannes Schindelin wrote:

Hi Igor,

On Thu, 23 Nov 2017, Igor Djordjevic wrote:


[ +Cc:  Git for Windows mailing list ]

I have no idea why it claimed that that group does not exist, the email
address looks correct to me.


On 23/11/2017 19:51, Phil Martel wrote:

I'm trying to install Git-2.15.0-64-bit.exe onto my Windows 10
machine.  When I run this installer program no matter what options I
try or whether I run as administrator it ends up with an error box
saying "The drive or UNC share you selected does not exist or is not
accessible. Please select another".  I do not see any way of
selecting a drive.  Any suggestions?

 From what I could Google around, this seems to be (Inno Setup?)
installation related issue...?

Indeed.


Do you already have "Git for Windows" installed? If so, does it work
if you try uninstalling it first?

That is a workaround, correct.


p.s. Note the existence of "Git for Windows"[1] specific mailing list
as well, where this issue might belong better.

[1] git-for-wind...@googlegroups.com

I think a much better place is the Git for Windows bug tracker (if you
ever wonder where the bug tracker is, or the home page, or the repository
or the FAQ, there are links in the upper left of the release notes --
which probably nobody reads, even if I really try to make them worth the
while -- and which you can find in C:\Program Files\Git\ReleaseNotes.html
if you closed the tab after installing Git for Windows).

And indeed, there is already a ticket for this issue:
https://github.com/git-for-windows/git/issues/1074

The original reporter did not respond to any questions, maybe you can do
better, Phil?
My case seems similar although it may be different.  I originally had 
Git installed on my C: drive.  A hardware upgrade moved that drive to 
E:.  I was able to link everything back and to run Git Bash (IIRC).


A few weeks later, the E: drive got sick.  The repair shop I went to was 
not able to restore much, but in the course of changes took my DVD drive 
out of the boot sequence which changed the E: drive to D:.  I was able 
to restore most of my files from an external herd disk backup, but the 
backup was from when the disk was C:


I could not run Git when I tried, so I downloaded the latest version 
from git-scm.com.  This failed to install with the  "The drive or UNC 
share you selected does not exist or is not accessible. Please select 
another" error.  I was puzzled because the installation code had not 
asked me to select a drive.  Following Buga's suggestion, I tried 
uninstalling Git.  I believe Windows said it could not find the 
uninstall information, but it did remove Git from the list of programs.  
I also removed Git from the start menu.  The installation then succeeded.


Best wishes,
--Phil


Ciao,
Johannes

Re: git status always modifies index?

[Jonathan Nieder]

Hi,

Jeff King wrote:
> 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 am worried that the project is not learning from what happened here.

My main issue with the --no-optional-locks name is that it does not
connect to the underlying user need.  Your main argument for it is
that it exactly describes the underlying user need.  One of us has to
be wrong.

So let me describe my naive reading:

As a user, I want to inspect the state of the repository without
disrupting it in any way.  That means not breaking concurrent
processes and not upsetting permissions.  --read-only seems to
describe this use case to me perfectly.

If I understood correctly, your objection is that --read-only is not
specific enough.  What I really want, you might say, is not to break
concurrent processes.  Any other aspects of being read-only are not
relevant.  E.g. if I can refresh the on-disk index using O_APPEND
without disrupting concurrent processes then I should be satisfied
with that.

Fair enough, though that feels like overengineering.  But I *still*
don't see what that has to do with the name "no-optional-locks".  When
is a lock *optional*?  And how am I supposed to discover this option?

This also came up during review, and I am worried that this review
feedback is being ignored.  In other words, I have no reason to
believe it won't happen again.

> I would be fine with having a further aspirational "read only" mode.

Excellent, we seem to agree on this much.  If I can find time for it
today then I'll write a patch.

Thanks,
Jonathan

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

[Thomas Gummerer]

On 11/27, Junio C Hamano wrote:
> 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.

Makes sense, I missed that.  I'll fix it in the re-roll.  I'll wait a
few days to see if there are any more comments on the series and then
re-roll it with the suggested changes.

Thanks for the review!

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

[Thomas Gummerer]

On 11/27, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Currently 'git worktree add ' creates a new branch named after the
> > basename of the , that matches the HEAD of whichever worktree we
> > were on when calling "git worktree add ".
> >
> > 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.

Sorry about that, will re-phrase.

> > +--[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.

As you mentioned below it's silently ignored.  The main reason for not
erroring out is that it would get a little bit (although not too much)
more annoying once the config variable is introduced.  If it's
strongly preferred to error out when  is given I can change it
to that.

Either way I'll update the documentation.

Thanks!

> > @@ -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: [PATCH] git-status.txt: mention --no-optional-locks

[Johannes Schindelin]

Hi Kaartic,

On Mon, 27 Nov 2017, Kaartic Sivaraam wrote:

> On Monday 27 November 2017 11:37 AM, Junio C Hamano wrote:
> > Jeff King  writes:
> > > +using `git --no-optional-locks status` (see linkgit:git[1] for details).
> 
> It strikes me just now that `--no-side-effects` might have been a better
> name for the option (of course, iff this avoid all kinds of side
> effects. I'm not sure about the side affects other than index refreshing
> of "git status"). And in case we didn't care about the predictability of
> option names even a little, `--do-what-i-say` might have been a catchy
> alternative ;-)

Your reasoning points to an important insight: while writing index.lock
files is a side effect of `git status`, and while there may be other side
effects in other operations, it is highly doubtful that any caller would
just want to switch them off wholesale. Instead, it is much more likely
that callers will want to pick the side effect they want to switch off.

Ciao,
Dscho

Re: git status always modifies index?

[Johannes Schindelin]

Hi Junio,

On Mon, 27 Nov 2017, Junio C Hamano wrote:

> 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.

The --no-optional-locks feature is

- hard to find,

- in the current scenarios less desirable than a very concrete "do not
  write index.lock files in `git status`",

- too simple to introduce to merit introducing it *before* a need for it
  arises that is larger than `git status --no-lock-index`, and you would
  still have to keep the latter because it is a very concrete and real use
  case that is unlikely to want to avoid other .lock files too.

So while you two are happily on agreeing with one another, the reality is
that this supposedly better design is nothing else than premature
optimization.

Ciao,
Dscho

Re: git status always modifies index?

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Mon, 27 Nov 2017, Jeff King wrote:

>> [...] IMHO it argues for GfW trying to land patches upstream first, and
>> then having them trickle in as you merge upstream releases.
>
> You know that I tried that, and you know why I do not do that anymore: it
> simply takes too long, and the review on the list focuses on things I
> cannot focus on as much, I need to make sure that the patches *work*
> first, whereas the patch review on the Git mailing list tends to ensure
> that they have the proper form first.
>
> I upstream patches when I have time.

You have been developing in the open, so no complaints from me, just a
second point of reference:

For Google's internal use we sometimes have needed a patch faster than
upstream can review it.  Our approach in those cases has been to send
a patch to the mailing list and then apply it internally immediately.
If upstream is stalled for months on review, so be it --- we already
have the patch.  But this tends to help ensure that we are moving in
the same direction.

That said, I don't think that was the main issue with
--no-optional-locks.  I'll comment more on that in another subthread.

Thanks,
Jonathan

Re: git status always modifies index?

[Johannes Schindelin]

Hi Peff,

On Mon, 27 Nov 2017, Jeff King wrote:

> [...] IMHO it argues for GfW trying to land patches upstream first, and
> then having them trickle in as you merge upstream releases.

You know that I tried that, and you know why I do not do that anymore: it
simply takes too long, and the review on the list focuses on things I
cannot focus on as much, I need to make sure that the patches *work*
first, whereas the patch review on the Git mailing list tends to ensure
that they have the proper form first.

I upstream patches when I have time.

Ciao,
Dscho

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[brian m. carlson]

On Mon, Nov 27, 2017 at 02:47:16PM +0100, lars.schnei...@autodesk.com wrote:
> From: Junio C Hamano 
> 
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user may be left staring at the
> original Git terminal window without even realizing that s/he needs to
> interact with another window before Git can proceed. To this user Git
> appears hanging.
> 
> Show a message in the original terminal and get rid of it when the
> editor returns.
> diff --git a/editor.c b/editor.c
> index 7519edecdc..4251ae9d7a 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, 
> const char *const *en
>   const char *args[] = { editor, real_path(path), NULL };
>   struct child_process p = CHILD_PROCESS_INIT;
>   int ret, sig;
> + static const char *close_notice = NULL;
> +
> + if (isatty(2) && !close_notice) {

Sorry for coming to the topic so late, but it occurred to me that we
might want to conditionalize this on an advice.* flag.  I expect there
are some people who will never want to see this, and letting them turn
it off would be good.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 27 Nov 2017, at 19:36, Eric Sunshine  wrote:
> 
> On Mon, Nov 27, 2017 at 8:47 AM,   wrote:
>> When a graphical GIT_EDITOR is spawned by a Git command that opens
>> and waits for user input (e.g. "git rebase -i"), then the editor window
>> might be obscured by other windows. The user may be left staring at the
>> original Git terminal window without even realizing that s/he needs to
>> interact with another window before Git can proceed. To this user Git
>> appears hanging.
>> 
>> Show a message in the original terminal and get rid of it when the
>> editor returns.
>> 
>> Signed-off-by: Junio C Hamano 
>> Signed-off-by: Lars Schneider 
>> ---
>> diff --git a/editor.c b/editor.c
>> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf 
>> *buffer, const char *const *en
>> +   static const char *close_notice = NULL;
>> +
>> +   if (isatty(2) && !close_notice) {
>> +   char *term = getenv("TERM");
>> +
>> +   if (term && strcmp(term, "dumb"))
>> +   /*
>> +* go back to the beginning and erase the
>> +* entire line if the terminal is capable
>> +* to do so, to avoid wasting the vertical
>> +* space.
>> +*/
>> +   close_notice = "\r\033[K";
>> +   else if (term && strstr(term, "emacsclient"))
> 
> You need to check 'editor' here, not 'term', and you should do it
> before the "not dumb" terminal check, otherwise you'll never get this
> far.

Ouch. That happens if I try to do two things at once. Embarrassing.

Thanks,
Lars


> 
>> +   /*
>> +* `emacsclient` (or `emacsclientw` on 
>> Windows) already prints
>> +* ("Waiting for Emacs...") if a file is 
>> opened for editing.
>> +* Therefore, we don't need to print the 
>> editor launch info.
>> +*/
>> +   ;
>> +   else
>> +   /* otherwise, complete and waste the line */
>> +   close_notice = _("done.\n");
>> +   }

[PATCH v2] diff: support anchoring line(s)

[Jonathan Tan]

Teach diff a new algorithm, one that attempts to prevent user-specified
lines from appearing as a deletion or addition in the end result. The
end user can use this by specifying "--anchored=" one or more
times when using Git commands like "diff" and "show".

Signed-off-by: Jonathan Tan 
---
In v2, the UI treats it as a new algorithm. (Internally it's still an
option for "patience".)

Stefan suggested that I add a test where a single --anchored=<> matches
multiple lines, and that reminded me that the patience algorithm only
really works on unique lines. I have updated the documentation to remark
on that, and also added a test to show what happens. I have also added a
test to show that later algorithm options override earlier ones.
---
 Documentation/diff-options.txt | 10 +
 diff.c | 22 +-
 diff.h |  4 ++
 t/t4064-diff-anchored.sh   | 94 ++
 xdiff/xdiff.h  |  4 ++
 xdiff/xpatience.c  | 42 ---
 6 files changed, 169 insertions(+), 7 deletions(-)
 create mode 100755 t/t4064-diff-anchored.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1..6ce39fb69 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -80,6 +80,16 @@ endif::git-format-patch[]
 --histogram::
Generate a diff using the "histogram diff" algorithm.
 
+--anchored=::
+   Generate a diff using the "anchored diff" algorithm.
++
+This option may be specified more than once.
++
+If a line exists in both the source and destination, exists only once,
+and starts with this text, this algorithm attempts to prevent it from
+appearing as a deletion or addition in the output. It uses the "patience
+diff" algorithm internally.
+
 --diff-algorithm={patience|minimal|histogram|myers}::
Choose a diff algorithm. The variants are as follows:
 +
diff --git a/diff.c b/diff.c
index 0763e8926..98aa638f8 100644
--- a/diff.c
+++ b/diff.c
@@ -3210,6 +3210,8 @@ static void builtin_diff(const char *name_a,
ecbdata.opt = o;
ecbdata.header = header.len ?  : NULL;
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xecfg.flags = XDL_EMIT_FUNCNAMES;
@@ -3302,6 +3304,8 @@ static void builtin_diffstat(const char *name_a, const 
char *name_b,
memset(, 0, sizeof(xpp));
memset(, 0, sizeof(xecfg));
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
if (xdi_diff_outf(, , diffstat_consume, diffstat,
@@ -4594,9 +4598,18 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, INDENT_HEURISTIC);
else if (!strcmp(arg, "--no-indent-heuristic"))
DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-   else if (!strcmp(arg, "--patience"))
+   else if (!strcmp(arg, "--patience")) {
+   int i;
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
-   else if (!strcmp(arg, "--histogram"))
+   /*
+* Both --patience and --anchored use PATIENCE_DIFF
+* internally, so remove any anchors previously
+* specified.
+*/
+   for (i = 0; i < options->anchors_nr; i++)
+   free(options->anchors[i]);
+   options->anchors_nr = 0;
+   } else if (!strcmp(arg, "--histogram"))
options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
else if ((argcount = parse_long_opt("diff-algorithm", av, ))) {
long value = parse_algorithm_value(optarg);
@@ -4608,6 +4621,11 @@ int diff_opt_parse(struct diff_options *options,
options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
options->xdl_opts |= value;
return argcount;
+   } else if (skip_prefix(arg, "--anchored=", )) {
+   options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
+   ALLOC_GROW(options->anchors, options->anchors_nr + 1,
+  options->anchors_alloc);
+   options->anchors[options->anchors_nr++] = xstrdup(arg);
}
 
/* flags options */
diff --git a/diff.h b/diff.h
index 0fb18dd73..7cf276f07 100644
--- a/diff.h
+++ b/diff.h
@@ -166,6 +166,10 @@ struct diff_options {
const char *stat_sep;
long xdl_opts;
 
+   /* see Documentation/diff-options.txt */
+   char **anchors;
+   size_t anchors_nr, anchors_alloc;
+
int stat_width;
int 

Re: [PATCH v5 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/22/2017 5:56 PM, Stefan Beller wrote:

On Tue, Nov 21, 2017 at 12:58 PM, Jeff Hostetler  wrote:

+   assert(arg);
+   assert(!unset);


I count 16 asserts in this patch. Is that really needed?
Either omit them or use BUG if we want to rely on user
bug reports when these conditions trigger, as assert is unreliable
due to its dependence on the NDEBUG flag.


Yes, there are a few asserts in the code.  Old habits

I could remove some/all of them, but personally I feel they
have merit and hint to the mindset of the author for future
readers of the code.  Are there other opinions?

Personally, I think it might be awkward to keep repeating
something like:

if (!c)
BUG(msg);

Do we want to think about a macro that builds on BUG() and
does the test?

Something like:
#define ASSERT_OR_BUG(c) do { if (!(c)) BUG("%s", #c); } while (0)

Jeff

Re: [PATCHv5 7/7] builtin/describe.c: describe a blob

[Stefan Beller]

On Thu, Nov 23, 2017 at 11:18 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Tue, Nov 21, 2017 at 11:55 PM, Junio C Hamano  wrote:
>>> Stefan Beller  writes:
>>>
 ...
 fixed.
 ...
 fixed the text...
 ...
 I am not fully convinced all descriptions are in recent history, but I
 tend to agree that most are, so probably the trade off is a wash.
>>>
>>> So what do we want with this topic?  I think the "teach 'git log' to
>>> highlight commits whose changes involve the given blob" is a more or
>>> less an orthogonal thing,
>>
>> Well, both of them solve our immediate needs, so I'd be fine with pursuing
>> just one of them, but I do not oppose taking both.
>>
>>> and I suspect that it is something users
>>> may (although I personally do not) find valuable to have a related
>>> but different feature in "git describe".
>>
>> agreed.
>
> I was reacting to your "fixed".  So will we see a rerolled series or
> not?

I was not planning on rerolling it.

FYI: I am on vacation until next Monday, so if there is anything
to be fixed here, I'd do it in a week, which may be enough time
to warrant incremental fixes?

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Eric Sunshine]

On Mon, Nov 27, 2017 at 8:47 AM,   wrote:
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user may be left staring at the
> original Git terminal window without even realizing that s/he needs to
> interact with another window before Git can proceed. To this user Git
> appears hanging.
>
> Show a message in the original terminal and get rid of it when the
> editor returns.
>
> Signed-off-by: Junio C Hamano 
> Signed-off-by: Lars Schneider 
> ---
> diff --git a/editor.c b/editor.c
> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, 
> const char *const *en
> +   static const char *close_notice = NULL;
> +
> +   if (isatty(2) && !close_notice) {
> +   char *term = getenv("TERM");
> +
> +   if (term && strcmp(term, "dumb"))
> +   /*
> +* go back to the beginning and erase the
> +* entire line if the terminal is capable
> +* to do so, to avoid wasting the vertical
> +* space.
> +*/
> +   close_notice = "\r\033[K";
> +   else if (term && strstr(term, "emacsclient"))

You need to check 'editor' here, not 'term', and you should do it
before the "not dumb" terminal check, otherwise you'll never get this
far.

> +   /*
> +* `emacsclient` (or `emacsclientw` on 
> Windows) already prints
> +* ("Waiting for Emacs...") if a file is 
> opened for editing.
> +* Therefore, we don't need to print the 
> editor launch info.
> +*/
> +   ;
> +   else
> +   /* otherwise, complete and waste the line */
> +   close_notice = _("done.\n");
> +   }

Re: [PATCH] xdiff/xpatience: support anchoring line(s)

[Jonathan Tan]

On Thu, 23 Nov 2017 11:47:02 +0900
Junio C Hamano  wrote:

> Thinking about this a bit more, I do like the basic idea of the UI
> even better.  What we could do is to sell this to the end users as a
> new kind of diff algorithm choice (i.e. myers, patience, ... will
> gain a new friend) that internally happens to be implemented by
> piggybacking on patience (just like minimal is piggybacking on
> myers) and call it "anchor".  Then just like this command line
> 
> git diff --histogram --patience
> 
> makes the last one win without complaint, it is sane that these
> command lines
> 
> git diff --histogram --anchored=
> git diff --anchored= --histogram
> 
> make the last one win without complaint, either.
> 
> Hmm?

This sounds good. There will be a bit of inconsistency in that in "git
diff --anchored= --anchored=", it is not the last
one that wins, but both of them will in fact be used. But I think that
in practice, this will be fine.

I'll send out another version with this UI (and with Stefan's test
suggestion).

[RFC PATCH v2] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

"git worktree remove" removes both the named worktree
directory and the administrative information for it after
checking that there is no local modifications that would be
lost (which is a handy safety measure). However, due to a
possible oversight, it aborts with an error if the worktree
directory is _already_ removed.

The user could use "git worktree prune" after seeing the
error and realizing the situation, but at that point, there
is nothing gained by leaving only the administrative data
behind. Teach "git worktree remove" to go ahead and remove
the trace of the worktree in such a case.

Helped-by: Eric Sunshine 
Signed-off-by: Kaartic Sivaraam 
---
 Changes in v2:

  - incorporated the suggestion to avoid quieting `validate_worktree()`
to detect inexistent directory (thanks, Eric!)

  - used the suggested (much better) commit message

 builtin/worktree.c | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b5afba164..6eab91889 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const 
char *prefix)
return update_worktree_location(wt, dst.buf);
 }
 
+/* Removes the .git/worktrees/worktree_id directory for
+ * the given worktree_id
+ *
+ * Returns 0 on success and non-zero value in case of failure
+ */
+static int remove_worktree_entry(char *worktree_id) {
+   int ret = 0;
+   struct strbuf we_path = STRBUF_INIT;
+   strbuf_addstr(_path, git_common_path("worktrees/%s", worktree_id));
+   if (remove_dir_recursively(_path, 0)) {
+   error_errno(_("failed to delete '%s'"), we_path.buf);
+   ret = -1;
+   }
+   strbuf_release(_path);
+   return ret;
+}
+
 static int remove_worktree(int ac, const char **av, const char *prefix)
 {
int force = 0;
@@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
die(_("already locked, reason: %s"), reason);
die(_("already locked, no reason"));
}
+
+   if (!file_exists(wt->path)) {
+   /* There's a worktree entry but the worktree directory
+* doesn't exist. So, just remove the worktree entry.
+*/
+   ret = remove_worktree_entry(wt->id);
+   free_worktrees(worktrees);
+   return ret;
+   }
+
if (validate_worktree(wt, 0))
return -1;
 
@@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
error_errno(_("failed to delete '%s'"), sb.buf);
ret = -1;
}
-   strbuf_reset();
-   strbuf_addstr(, git_common_path("worktrees/%s", wt->id));
-   if (remove_dir_recursively(, 0)) {
-   error_errno(_("failed to delete '%s'"), sb.buf);
-   ret = -1;
-   }
-   strbuf_release();
+   ret = remove_worktree_entry(wt->id);
free_worktrees(worktrees);
return ret;
 }
-- 
2.15.0.345.gf926f18f3

Re: git checkout's stderr v stdout usage

[rio45]

Great succession! 
I appreciates you and good to see such a positive responses. 
You can change your ip address of  192.168.l.254 login
   and password details at anytime. 
Thank you!!



--
Sent from: http://git.661346.n2.nabble.com/

[PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out sytax is used with
'--branch' option of check-ref-format the results might not
always be a valid branch name as @{-N} is used to refer to
the N-th last checked out "thing" which might be any commit
(sometimes a branch). The documentation thus does a wrong thing
by promoting it as the "previous branch syntax".

So, correctly state @{-N} is the syntax for specifying "N-th last
thing checked out" and also state that the result of using @{-N}
might also result in a "commit hash".

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-check-ref-format.txt | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit hash when the N-th last thing checked out was not a branch.
 
 OPTIONS
 ---
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.345.gf926f18f3

[PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state

[Kaartic Sivaraam]

@{-N} is a syntax for the N-th last "checkout" and not the N-th
last "branch". Therefore, in some cases using `git checkout @{-$N}`
DOES lead to a "detached HEAD" state. This can also be ensured by
the commit message of 75d6e552a (Documentation: @{-N} can refer to
a commit, 2014-01-19) which clearly specifies how @{-N} can be used
to refer not only to a branch but also to a commit.

Correct the misleading sentence which states that @{-N} doesn't
detach HEAD.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-checkout.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e108b0f74..d5a57ac90 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -272,11 +272,11 @@ section of linkgit:git-add[1] to learn how to operate the 
`--patch` mode.
commit, your HEAD becomes "detached" and you are no longer on
any branch (see below for details).
 +
-As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
-checks out branches (instead of detaching).  You may also specify
-`-` which is synonymous with `"@{-1}"`.
+You can use the `"@{-N}"` syntax to refer to the N-th last
+branch/commit checked out using "git checkout" operation. You may
+also specify `-` which is synonymous to `"@{-1}`.
 +
-As a further special case, you may use `"A...B"` as a shortcut for the
+As a special case, you may use `"A...B"` as a shortcut for the
 merge base of `A` and `B` if there is exactly one merge base. You can
 leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
-- 
2.15.0.345.gf926f18f3

[PATCH v2 2/3] rebase: distinguish user input by quoting it

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam 
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index a675cf691..3f8d99e99 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -477,7 +477,7 @@ then
;;
esac
upstream=$(peel_committish "${upstream_name}") ||
-   die "$(eval_gettext "invalid upstream \$upstream_name")"
+   die "$(eval_gettext "invalid upstream '\$upstream_name'")"
upstream_arg="$upstream_name"
 else
if test -z "$onto"
@@ -540,7 +540,7 @@ case "$#" in
head_name="detached HEAD"
 
else
-   die "$(eval_gettext "fatal: no such branch/commit: 
\$branch_or_commit")"
+   die "$(eval_gettext "fatal: no such branch/commit 
'\$branch_or_commit'")"
fi
;;
 0)
-- 
2.15.0.345.gf926f18f3

[PATCH v2 3/3] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

Attempting to rebase when the HEAD is detached and is already
up to date with upstream (so there's nothing to do), the
following message is shown

Current branch HEAD is up to date.

which is clearly wrong as HEAD is not a branch.

Handle the special case of HEAD correctly to give a more precise
error message.

Signed-off-by: Kaartic Sivaraam 
---

 git-rebase.sh | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 3f8d99e99..9cce1483a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -602,11 +602,23 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date.")"
+   if test "$branch_or_commit" = "HEAD" &&
+!(git symbolic-ref -q HEAD)
+   then
+   say "$(eval_gettext "HEAD is up to date.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date.")"
+   fi
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date, rebase forced.")"
+   if test "$branch_or_commit" = "HEAD" &&
+!(git symbolic-ref -q HEAD)
+   then
+   say "$(eval_gettext "HEAD is up to date, rebase 
forced.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date, rebase forced.")"
+   fi
fi
 fi
 
-- 
2.15.0.345.gf926f18f3

[PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

Junio C Hamano  writes:
> Perhaps time to learn "git symbolic-ref HEAD" and use it instead of
> depending on the name?

Good point. Helped remove the assumption that there's no branch named HEAD.
(and indirectly led to 2 additional patches and the 3 questions found below ;-)


This started as a small fix to make 'git rebase' give a precise
error message when a rebase was done with a detached HEAD. Now it
includes a few cleanups that I caught while analysing the code.

There were a few weird observations when I was anlaysing the code.
They are listed below. Please give your thoughts about them.

The commands I use below were run on my local clone of 'git' and 'origin'
in this context refers to the git mirror at GitHub.

1. "git rebase  " does nothing

I tried to play around with rebase by trying out various combinations while
analysing and found the following to have not effect even though the output
doesn't say anything about that,

$ git rebase origin/next origin/maint 
First, rewinding head to replay your work on top of it...
Fast-forwarded origin/maint to origin/next.

IOW, updating a remote branch with a remote upstream had no effect.
Though trying to update a remote branch with a remote upstream doesn't
seem to be very meaningful, the output says it HAS updated the remote
which seems to be misleading. What should be done about this?

2. It's possible to do "git rebase  "

$ git origin/next f926f18f3dda0c52e794b2de0911f1b046c7dadf"

This checks out the commit(detaches HEAD) tries to rebase origin/next
from there.

This behaviour doesn't seems to be documented. It says that only a 'branch'
can be specified. (The error message updated in 1/3 previously reported that
the 'branch' name is invalid rather than stating the 'ref (branch/commit) is
invlid')

 git rebase [...] [ []]
 git rebase [...] --root []
 ...

Shouldn't it have said that we can give any  apart from  instead of
saying we could give only a . If intentional, why?

3. "git rebase  " shows misleading message

$ git origin/next f926f18f3dda0c52e794b2de0911f1b046c7dadf"
Current branch f926f18f3dda0c52e794b2de0911f1b046c7dadf is up to date.

As it's clear the commit is not a branch. What should be done to fix this?


Kaartic Sivaraam (3):
  rebase: use a more appropriate variable name
  rebase: distinguish user input by quoting it
  rebase: rebasing can also be done when HEAD is detached

 git-rebase.sh | 46 +++---
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.15.0.345.gf926f18f3

[PATCH v2 1/3] rebase: use a more appropriate variable name

[Kaartic Sivaraam]

The variable name "branch_name" used to indicate the  parameter
in "git rebase  " is misleading as it seems to indicate
that it holds only a "branch name" although at times it might actually hold
any valid  (branch/commit).

So, use a more suitable name for that variable.

Signed-off-by: Kaartic Sivaraam 
---
 git-rebase.sh | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6344e8d5e..a675cf691 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -518,36 +518,40 @@ case "$onto_name" in
 esac
 
 # If the branch to rebase is given, that is the branch we will rebase
-# $branch_name -- branch being rebased, or HEAD (already detached)
+# $branch_or_commit -- branch/commit being rebased, or HEAD (already detached)
 # $orig_head -- commit object name of tip of the branch before rebasing
 # $head_name -- refs/heads/ or "detached HEAD"
 switch_to=
 case "$#" in
 1)
# Is it "rebase other $branchname" or "rebase other $commit"?
-   branch_name="$1"
+   branch_or_commit="$1"
switch_to="$1"
 
-   if git show-ref --verify --quiet -- "refs/heads/$1" &&
-  orig_head=$(git rev-parse -q --verify "refs/heads/$1")
+   # Is it a local branch?
+   if git show-ref --verify --quiet -- "refs/heads/$branch_or_commit" &&
+  orig_head=$(git rev-parse -q --verify "refs/heads/$branch_or_commit")
then
-   head_name="refs/heads/$1"
-   elif orig_head=$(git rev-parse -q --verify "$1")
+   head_name="refs/heads/$branch_or_commit"
+
+   # If not is it a valid ref (branch or commit)?
+   elif orig_head=$(git rev-parse -q --verify "$branch_or_commit")
then
head_name="detached HEAD"
+
else
-   die "$(eval_gettext "fatal: no such branch: \$branch_name")"
+   die "$(eval_gettext "fatal: no such branch/commit: 
\$branch_or_commit")"
fi
;;
 0)
# Do not need to switch branches, we are already on it.
-   if branch_name=$(git symbolic-ref -q HEAD)
+   if branch_or_commit=$(git symbolic-ref -q HEAD)
then
-   head_name=$branch_name
-   branch_name=$(expr "z$branch_name" : 'zrefs/heads/\(.*\)')
+   head_name=$branch_or_commit
+   branch_or_commit=$(expr "z$branch_or_commit" : 
'zrefs/heads/\(.*\)')
else
head_name="detached HEAD"
-   branch_name=HEAD ;# detached
+   branch_or_commit="HEAD"
fi
orig_head=$(git rev-parse --verify HEAD) || exit
;;
@@ -598,11 +602,11 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date.")"
+   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date.")"
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date, rebase forced.")"
+   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date, rebase forced.")"
fi
 fi
 
@@ -632,7 +636,7 @@ git update-ref ORIG_HEAD $orig_head
 # we just fast-forwarded.
 if test "$mb" = "$orig_head"
 then
-   say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
+   say "$(eval_gettext "Fast-forwarded \$branch_or_commit to 
\$onto_name.")"
move_to_original_branch
finish_rebase
exit 0
-- 
2.15.0.345.gf926f18f3

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Monday 27 November 2017 07:17 PM, lars.schnei...@autodesk.com wrote:


Show a message in the original terminal and get rid of it when the
editor returns.



"... except in the case when an error occurs." could  be included if needed.

+   static const char *close_notice = NULL;
+


IIRC, this variable is bound to be `static` for sake of future proofing. 
So, I guess you could use the following suggestion of Eric Sunshine in 
the below conditional.


If you reverse this condition to say (!close_notice && isatty(2)),
then you save an isatty() invocation each time if close_notice is
already assigned.


+   if (isatty(2) && !close_notice) {
+   char *term = getenv("TERM");
+
+   if (term && strcmp(term, "dumb"))
+   /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+   close_notice = "\r\033[K";
+   else if (term && strstr(term, "emacsclient"))
+   /*
+* `emacsclient` (or `emacsclientw` on Windows) 
already prints
+* ("Waiting for Emacs...") if a file is opened 
for editing.
+* Therefore, we don't need to print the editor 
launch info.
+*/
+   ;
+   else
+   /* otherwise, complete and waste the line */
+   close_notice = _("done.\n");
+   }
+
+   if (close_notice) {
+   fprintf(stderr, _("Launched editor. Waiting for your 
input... "));
+   fflush(stderr);
+   }

p.argv = args;
p.env = env;
@@ -53,11 +82,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+   if (close_notice)
+   fputs(close_notice, stderr);
}

if (!buffer)
--
2.15.0

[PATCH v3 1/4] Makefile: generate Perl header from template file

[Dan Jacques]

Currently, the generated Perl script headers are emitted by commands in
the Makefile. This mechanism restricts options to introduce alternative
header content, needed by Perl runtime prefix support, and obscures the
origin of the Perl script header.

Change the Makefile to generate a header by processing a template file and
move the header content into the "perl/" subdirectory. The processed
generated will now be stored in the "GIT-PERL-HEADER" file. This allows
the content of the Perl header to be controlled by changing the path of
the template in the Makefile.

Signed-off-by: Dan Jacques 
---
 .gitignore   |  1 +
 Makefile | 23 ++-
 perl/header_fixed_prefix.pl.template |  1 +
 3 files changed, 16 insertions(+), 9 deletions(-)
 create mode 100644 perl/header_fixed_prefix.pl.template

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..89bd7bd8a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@
 /GIT-LDFLAGS
 /GIT-PREFIX
 /GIT-PERL-DEFINES
+/GIT-PERL-HEADER
 /GIT-PYTHON-VARS
 /GIT-SCRIPT-DEFINES
 /GIT-USER-AGENT
diff --git a/Makefile b/Makefile
index e53750ca0..5ad60a54c 100644
--- a/Makefile
+++ b/Makefile
@@ -1964,18 +1964,15 @@ perl/PM.stamp: FORCE
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' 
prefix='$(prefix_SQ)' $(@F)
 
+PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
-$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
+
+$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER 
GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ && \
-   INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
instlibdir` && \
-   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
-   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e '1{' \
-e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
-   -e 'h' \
-   -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || 
"'"$$INSTLIBDIR"'"));=' \
-   -e 'H' \
-   -e 'x' \
+   -e 'rGIT-PERL-HEADER' \
+   -e 'G' \
-e '}' \
-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
$< >$@+ && \
@@ -1989,6 +1986,14 @@ GIT-PERL-DEFINES: FORCE
echo "$$FLAGS" >$@; \
fi
 
+GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak
+   $(QUIET_GEN)$(RM) $@ && \
+   INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
instlibdir` && \
+   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+   sed -e 's=@@PATHSEP@@='$(pathsep)'=g' \
+   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+   $< >$@
 
 .PHONY: gitweb
 gitweb:
@@ -2707,7 +2712,7 @@ ifndef NO_TCLTK
 endif
$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
$(RM) GIT-USER-AGENT GIT-PREFIX
-   $(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PYTHON-VARS
+   $(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER 
GIT-PYTHON-VARS
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
diff --git a/perl/header_fixed_prefix.pl.template 
b/perl/header_fixed_prefix.pl.template
new file mode 100644
index 0..9a4bc4d30
--- /dev/null
+++ b/perl/header_fixed_prefix.pl.template
@@ -0,0 +1 @@
+use lib (split(/@@PATHSEP@@/, $ENV{GITPERLLIB} || "@@INSTLIBDIR@@"));
-- 
2.15.0.chromium12

[PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Dan Jacques]

Enable Git to resolve its own binary location using a variety of
OS-specific and generic methods, including:

- procfs via "/proc/self/exe" (Linux)
- _NSGetExecutablePath (Darwin)
- KERN_PROC_PATHNAME sysctl on BSDs.
- argv0, if absolute (all, including Windows).

This is used to enable RUNTIME_PREFIX support for non-Windows systems,
notably Linux and Darwin. When configured with RUNTIME_PREFIX, Git will
do a best-effort resolution of its executable path and automatically use
this as its "exec_path" for relative helper and data lookups, unless
explicitly overridden.

Small incidental formatting cleanup of "exec_cmd.c".

Signed-off-by: Dan Jacques 
---
 Makefile |  35 +++-
 cache.h  |   1 +
 common-main.c|   4 +-
 config.mak.uname |   7 ++
 exec_cmd.c   | 239 +++
 exec_cmd.h   |   4 +-
 gettext.c|   8 +-
 git.c|   2 +-
 8 files changed, 260 insertions(+), 40 deletions(-)

diff --git a/Makefile b/Makefile
index a36815b49..014988318 100644
--- a/Makefile
+++ b/Makefile
@@ -416,6 +416,16 @@ all::
 #
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
+# Define HAVE_BSD_KERN_PROC_SYSCTL if your platform supports the KERN_PROC BSD
+# sysctl function.
+#
+# Define PROCFS_EXECUTABLE_PATH if your platform mounts a "procfs" filesystem
+# capable of resolving the path of the current executable. If defined, this
+# must be the canonical path for the "procfs" current executable path.
+#
+# Define HAVE_NS_GET_EXECUTABLE_PATH if your platform supports calling
+# _NSGetExecutablePath to retrieve the path of the running executable.
+#
 # Define HAVE_GETDELIM if your system has the getdelim() function.
 #
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
@@ -426,6 +436,12 @@ all::
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
 #
+# Define RUNTIME_PREFIX to configure Git to resolve its ancillary tooling and
+# support files relative to the location of the runtime binary, rather than
+# hard-coding them into the binary. Git installations built with RUNTIME_PREFIX
+# can be moved to arbitrary filesystem locations. Users may want to enable
+# RUNTIME_PREFIX_PERL as well (see below).
+#
 # Define RUNTIME_PREFIX_PERL to configure Git's PERL commands to locate Git
 # support libraries relative to their filesystem path instead of hard-coding
 # it.
@@ -466,6 +482,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   localedir
 #   perllibdir
 # This can help installing the suite in a relocatable way.
 
@@ -491,6 +508,7 @@ mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
 sharedir_relative = $(patsubst $(prefix)/%,%,$(sharedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
+localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir
 
@@ -1637,10 +1655,23 @@ ifdef HAVE_BSD_SYSCTL
BASIC_CFLAGS += -DHAVE_BSD_SYSCTL
 endif
 
+ifdef HAVE_BSD_KERN_PROC_SYSCTL
+   BASIC_CFLAGS += -DHAVE_BSD_KERN_PROC_SYSCTL
+endif
+
 ifdef HAVE_GETDELIM
BASIC_CFLAGS += -DHAVE_GETDELIM
 endif
 
+ifneq ($(PROCFS_EXECUTABLE_PATH),)
+   procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH))
+   BASIC_CFLAGS += 
'-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"'
+endif
+
+ifdef HAVE_NS_GET_EXECUTABLE_PATH
+   BASIC_CFLAGS += -DHAVE_NS_GET_EXECUTABLE_PATH
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
@@ -1723,6 +1754,7 @@ bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
 infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 localedir_SQ = $(subst ','\'',$(localedir))
+localedir_relative_SQ = $(subst ','\'',$(localedir_relative))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
@@ -2184,6 +2216,7 @@ endif
 exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+   '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
'-DBINDIR="$(bindir_relative_SQ)"' \
'-DPREFIX="$(prefix_SQ)"'
 
@@ -2201,7 +2234,7 @@ attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 
 gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
-   -DGIT_LOCALE_PATH='"$(localedir_SQ)"'
+   -DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"'
 
 http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS 
+= \
-DCURL_DISABLE_TYPECHECK
diff --git a/cache.h b/cache.h
index 2e1434505..3d84aa0bf 100644
--- a/cache.h
+++ b/cache.h
@@ -449,6 +449,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define 

[PATCH v3 2/4] Makefile: add support for "perllibdir"

[Dan Jacques]

Add the "perllibdir" Makefile variable, which allows the customization
of the Perl library installation path.

The Perl library installation path is currently left entirely to the
Perl Makefile implementation, either MakeMaker (default) or a fixed path
when NO_PERL_MAKEMAKER is enabled. This patch introduces "perllibdir", a
Makefile variable that can override that Perl module installation path.

As with some other Makefile variables, "perllibdir" may be either
absolute or relative. In the latter case, it is treated as relative to
"$(prefix)".

Add some incidental documentation to perl/Makefile.

Explicitly specifying an installation path is necessary for Perl runtime
prefix support, as runtime prefix resolution code must know in advance
where the Perl support modules are installed.

Signed-off-by: Dan Jacques 
---
 Makefile  | 18 +-
 perl/Makefile | 52 ++--
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 5ad60a54c..7a2ed8857 100644
--- a/Makefile
+++ b/Makefile
@@ -462,6 +462,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   perllibdir
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)
@@ -1721,6 +1722,7 @@ gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
+perllibdir_SQ = $(subst ','\'',$(perllibdir))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
@@ -1955,17 +1957,22 @@ $(SCRIPT_PERL_GEN): perl/perl.mak
 
 perl/perl.mak: perl/PM.stamp
 
-perl/PM.stamp: FORCE
+perl/PM.stamp: GIT-PERL-DEFINES FORCE
@$(FIND) perl -type f -name '*.pm' | sort >$@+ && \
+   cat GIT-PERL-DEFINES >>$@+ && \
$(PERL_PATH) -V >>$@+ && \
{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
$(RM) $@+
 
-perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
-   $(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' 
prefix='$(prefix_SQ)' $(@F)
-
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
+
+PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER)
+PERL_DEFINES += $(perllibdir)
+
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
+   $(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' \
+ prefix='$(prefix_SQ)' perllibdir='$(perllibdir_SQ)' $(@F)
 
 $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER 
GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ && \
@@ -1979,6 +1986,7 @@ $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak 
GIT-PERL-DEFINES GIT-PERL-HEADER GI
chmod +x $@+ && \
mv $@+ $@
 
+PERL_DEFINES := $(subst $(space),:,$(PERL_DEFINES))
 GIT-PERL-DEFINES: FORCE
@FLAGS='$(PERL_DEFINES)'; \
if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
diff --git a/perl/Makefile b/perl/Makefile
index f657de20e..b2aeeb0d8 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -1,6 +1,22 @@
 #
 # Makefile for perl support modules and routine
 #
+# This Makefile generates "perl.mak", which contains the actual build and
+# installation directions.
+#
+# PERL_PATH must be defined to be the path of the Perl interpreter to use.
+#
+# prefix must be defined as the Git installation prefix.
+#
+# localedir must be defined as the path to the locale data.
+#
+# perllibdir may be optionally defined to override the default Perl module
+# installation directory, which is relative to prefix. If perllibdir is not
+# absolute, it will be treated as relative to prefix.
+#
+# NO_PERL_MAKEMAKER may be defined to use a built-in Makefile generation method
+# instead of Perl MakeMaker.
+
 makfile:=perl.mak
 modules =
 
@@ -12,6 +28,16 @@ ifndef V
QUIET = @
 endif
 
+# If a library directory is provided, and it is not an absolute path, resolve
+# it relative to prefix.
+ifneq ($(perllibdir),)
+ifneq ($(filter /%,$(firstword $(perllibdir))),)
+perllib_instdir = $(perllibdir)
+else
+perllib_instdir = $(prefix)/$(perllibdir)
+endif
+endif
+
 all install instlibdir: $(makfile)
$(QUIET)$(MAKE) -f $(makfile) $@
 
@@ -25,7 +51,12 @@ clean:
 $(makfile): PM.stamp
 
 ifdef NO_PERL_MAKEMAKER
-instdir_SQ = $(subst ','\'',$(prefix)/lib)
+
+ifeq ($(perllib_instdir),)
+perllib_instdir = $(prefix)/lib
+endif
+
+instdir_SQ = $(subst ','\'',$(perllib_instdir))
 
 modules += Git
 modules += Git/I18N
@@ -42,7 +73,7 @@ modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
 modules += Git/SVN/Utils
 
-$(makfile): ../GIT-CFLAGS Makefile
+$(makfile): ../GIT-CFLAGS ../GIT-PERL-DEFINES Makefile
echo all: private-Error.pm Git.pm Git/I18N.pm > $@
set -e; \
for i in $(modules); \
@@ -79,12 +110,21 @@ $(makfile): ../GIT-CFLAGS Makefile
 

[PATCH v3 3/4] Makefile: add Perl runtime prefix support

[Dan Jacques]

Add a new Makefile flag, RUNTIME_PREFIX_PERL, which, when enabled,
configures Perl scripts to locate the Git installation's Perl support
libraries by resolving against the script's path, rather than
hard-coding that path at build-time.

Enabling RUNTIME_PREFIX_PERL overrides the system-specific Perl script
installation path generated by MakeMaker to force installation into a
platform-neutral location, "/share/perl5".

This change enables Git's Perl scripts to work when their Git installation
is relocated or moved to another system.

Signed-off-by: Dan Jacques 
---
 Makefile   | 40 +-
 perl/header_runtime_prefix.pl.template | 24 
 2 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 perl/header_runtime_prefix.pl.template

diff --git a/Makefile b/Makefile
index 7a2ed8857..a36815b49 100644
--- a/Makefile
+++ b/Makefile
@@ -425,6 +425,10 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define RUNTIME_PREFIX_PERL to configure Git's PERL commands to locate Git
+# support libraries relative to their filesystem path instead of hard-coding
+# it.
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -485,6 +489,7 @@ pathsep = :
 
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+sharedir_relative = $(patsubst $(prefix)/%,%,$(sharedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir
@@ -1967,7 +1972,38 @@ perl/PM.stamp: GIT-PERL-DEFINES FORCE
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
 
 PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
-PERL_DEFINES += $(NO_PERL_MAKEMAKER)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER) $(RUNTIME_PREFIX_PERL)
+
+# Support Perl runtime prefix. In this mode, a different header is installed
+# into Perl scripts. This header expects both the scripts and their support
+# library to be installed relative to $(prefix), and resolves the path to
+# the Perl libraries (perllibdir) from the executable's current path
+# (gitexecdir).
+#
+# This configuration requires both $(perllibdir) and $(gitexecdir) to be
+# relative paths, and will error if this is not the case.
+ifdef RUNTIME_PREFIX_PERL
+
+PERL_HEADER_TEMPLATE = perl/header_runtime_prefix.pl.template
+PERL_DEFINES += $(gitexecdir)
+
+# RUNTIME_PREFIX_PERL requires a $(perllibdir) value.
+ifeq ($(perllibdir),)
+perllibdir = $(sharedir_relative)/perl5
+endif
+
+ifneq ($(filter /%,$(firstword $(gitexecdir))),)
+$(error RUNTIME_PREFIX_PERL requires a relative $(gitexecdir))
+endif
+gitexecdir_relative_SQ = $(gitexecdir_SQ)
+
+ifneq ($(filter /%,$(firstword $(perllibdir))),)
+$(error RUNTIME_PREFIX_PERL requires a relative perllibdir)
+endif
+perllibdir_relative_SQ = $(perllibdir_SQ)
+
+endif
+
 PERL_DEFINES += $(perllibdir)
 
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
@@ -2001,6 +2037,8 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES 
perl/perl.mak
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e 's=@@PATHSEP@@='$(pathsep)'=g' \
-e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+   -e 's=@@GITEXECDIR@@='$(gitexecdir_relative_SQ)'=g' \
+   -e 's=@@PERLLIBDIR@@='$(perllibdir_relative_SQ)'=g' \
$< >$@
 
 .PHONY: gitweb
diff --git a/perl/header_runtime_prefix.pl.template 
b/perl/header_runtime_prefix.pl.template
new file mode 100644
index 0..fb9a9924d
--- /dev/null
+++ b/perl/header_runtime_prefix.pl.template
@@ -0,0 +1,24 @@
+# BEGIN RUNTIME_PREFIX_PERL generated code.
+#
+# This finds our Git::* libraries relative to the script's runtime path.
+BEGIN {
+   use lib split /@@PATHSEP@@/,
+   (
+   $ENV{GITPERLLIB}
+   ||
+   do {
+   require FindBin;
+   require File::Spec;
+   my $gitexecdir_relative = '@@GITEXECDIR@@';
+   my $perllibdir_relative = '@@PERLLIBDIR@@';
+
+   ($FindBin::Bin =~ m=${gitexecdir_relative}$=) ||
+   die('Unrecognized runtime path.');
+   my $prefix = substr($FindBin::Bin, 0, 
-length($gitexecdir_relative));
+   my $perllibdir = File::Spec->catdir($prefix, 
$perllibdir_relative);
+   (-e $perllibdir) || die("Invalid library path: 
$perllibdir");
+   $perllibdir;
+   }
+   );
+}
+# END RUNTIME_PREFIX_PERL generated code.
-- 
2.15.0.chromium12

[PATCH v3 0/4] RUNTIME_PREFIX relocatable Git

[Dan Jacques]

Previous threads:
v1: https://public-inbox.org/git/20171116170523.28696-1-...@google.com/
v2: https://public-inbox.org/git/20171119173141.4896-1-...@google.com/

After working with avarab@, I isolated the Perl changes into a separate
set of patches and adapted the code to be more correct and readable. I
opted to prescribe a relative Perl library path instead of letting
MakeMaker or the Config module choose one, since the latter both
incorporate build system parameters and a major purpose of this is to be
portable between ABI-compatible systems.

I've tested this via Travis and run full test suite with and without
RUNTIME_PREFIX/RUNTIME_PREFIX_PERL, and tested locally on Mac, Linux,
and FreeBSD systems. Please take a look!

Built using this "config.mak":

=== BEGIN config.mak ===

RUNTIME_PREFIX = YesPlease
RUNTIME_PREFIX_PERL = YesPlease
gitexecdir = libexec/git-core
template_dir = share/git-core/templates
sysconfdir = etc

=== END config.mak ===

Changes in v3 from v2:

- Broken into multiple patches now that Perl is isolated in its own
  RUNTIME_PREFIX_PERL flag.

- Working with avarab@, several changes to Perl script runtime prefix
  support:

  - Moved Perl header body content from Makefile into external template
file(s).

  - Added generic "perllibdir" variable to override Perl installation
path.

  - RUNTIME_PREFIX_PERL generated script header is more descriptive and
consistent with how the C version operates.

  - Fixed Generated Perl header Makefile dependency, should rebuild
when dependent files and flags change.

- Changed some of the new RUNTIME_PREFIX trace strings to use consistent
  formatting and terminology.

Changes in v2 from v1:

- Added comments and formatting to improve readability of
  platform-sepecific executable path resolution sleds in
  `git_get_exec_path`.

- Consolidated "cached_exec_path" and "argv_exec_path" globals
  into "exec_path_value".

- Use `strbuf_realpath` instead of `realpath` for procfs resolution.

- Removed new environment variable exports. Git with RUNTIME_PREFIX no
  longer exports or consumes any additional environment information.

- Updated Perl script resolution strategy: rather than having Git export
  the relative executable path to the Perl scripts, they now resolve
  it independently when RUNTIME_PREFIX_PERL is enabled.

- Updated resolution strategy for "gettext()": use system_path() instead
  of special environment variable.

- Added `sysctl` executable resolution support for BSDs that don't
  mount "procfs" by default (most of them).

Dan Jacques (4):
  Makefile: generate Perl header from template file
  Makefile: add support for "perllibdir"
  Makefile: add Perl runtime prefix support
  exec_cmd: RUNTIME_PREFIX on some POSIX systems

 .gitignore |   1 +
 Makefile   | 110 +--
 cache.h|   1 +
 common-main.c  |   4 +-
 config.mak.uname   |   7 +
 exec_cmd.c | 239 -
 exec_cmd.h |   4 +-
 gettext.c  |   8 +-
 git.c  |   2 +-
 perl/Makefile  |  52 ++-
 perl/header_fixed_prefix.pl.template   |   1 +
 perl/header_runtime_prefix.pl.template |  24 
 12 files changed, 395 insertions(+), 58 deletions(-)
 create mode 100644 perl/header_fixed_prefix.pl.template
 create mode 100644 perl/header_runtime_prefix.pl.template

-- 
2.15.0.chromium12

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

[Elijah Newren]

[Removed cc's that just bounce]

On Sun, Nov 26, 2017 at 7:40 PM, Junio C Hamano  wrote:
> 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

>>
>> 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.

Well, then perhaps I was wrong to submit it independent of my
directory rename series.  As noted in the (very lengthy) extended
commit message explanation, the assumption the previous code made just
happened to work until a few extra tweaks (from directory renames)
caused us to want to remove a file from the working copy that was
found at stage 0 in the index.  Thus, the only testcase we can really
use for this commit, is testcase 7b of the new t6043 added by that
other patch series, and it's only valid with the code from that other
series.

When I submitted this patch, I was thinking about just including this
fix with the next reroll of my rename-directory-detection series but
it partially felt like an independent fix...but maybe I chose wrong.

Would you prefer I include it in my next en/rename-directory-detection reroll?

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

[Jeff King]

On Sun, Nov 26, 2017 at 09:57:14PM +0100, Christian Couder wrote:

> > 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.

I guess I can buy that line of reasoning (and in particular your later
comment that autoconf is doomed to bitrot, since most of us don't use
it). I'm just slightly concerned that we're going to end up
reimplementing autoconf inside our Makefile. ;)

But that is a slippery slope argument; we could switch our approach any
time if it starts to look bad.

-Peff

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

[Jeff King]

On Mon, Nov 27, 2017 at 09:24:47AM +0100, Christian Couder 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 think any part of my patch should be controversial. I
> repeatedly wrote very long messages to show all the possible cases, so
> that it is easy to see that we are not worse in any case. And all the
> competing suggestions, even the above from Junio either have
> significant problems or address a different problem.

Sorry, controversial may not have been the right word. It's just that
the review discussion focused around packagers who may want to build git
without having "wish" on the build machine. If everybody is happy with
the BYPASS mechanism you added to address that, then I'm perfectly fine
with it.

-Peff

[PATCH v3] launch_editor(): indicate that Git waits for user input

[lars . schneider]

From: Junio C Hamano 

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user may be left staring at the
original Git terminal window without even realizing that s/he needs to
interact with another window before Git can proceed. To this user Git
appears hanging.

Show a message in the original terminal and get rid of it when the
editor returns.

Signed-off-by: Junio C Hamano 
Signed-off-by: Lars Schneider 
---


Hi,

diff to v2:
- shortened and localized the "waiting" message
- detect "emacsclient" and suppress "waiting" message

I did not add a newline after the "waiting" message in case of an error.
Junio indicated [1] that it is not necessary with the shorter "waiting"
message.

Thanks,
Lars


[1] https://public-inbox.org/git/xmqqk1ygnpu7@gitster.mtv.corp.google.com/

Notes:
Base Commit: 89ea799ffc (89ea799ffcc5c8a0547d3c9075eb979256ee95b8)
Diff on Web: https://github.com/larsxschneider/git/commit/0d43814931
Checkout:git fetch https://github.com/larsxschneider/git editor-v3 && 
git checkout 0d43814931

 editor.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/editor.c b/editor.c
index 7519edecdc..4251ae9d7a 100644
--- a/editor.c
+++ b/editor.c
@@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+   static const char *close_notice = NULL;
+
+   if (isatty(2) && !close_notice) {
+   char *term = getenv("TERM");
+
+   if (term && strcmp(term, "dumb"))
+   /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+   close_notice = "\r\033[K";
+   else if (term && strstr(term, "emacsclient"))
+   /*
+* `emacsclient` (or `emacsclientw` on Windows) 
already prints
+* ("Waiting for Emacs...") if a file is opened 
for editing.
+* Therefore, we don't need to print the editor 
launch info.
+*/
+   ;
+   else
+   /* otherwise, complete and waste the line */
+   close_notice = _("done.\n");
+   }
+
+   if (close_notice) {
+   fprintf(stderr, _("Launched editor. Waiting for your 
input... "));
+   fflush(stderr);
+   }

p.argv = args;
p.env = env;
@@ -53,11 +82,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+   if (close_notice)
+   fputs(close_notice, stderr);
}

if (!buffer)
--
2.15.0

Re: git-p4: cloning with a change number does not import all files

[Lars Schneider]

> On 25 Nov 2017, at 21:35, Patrick Rouleau  wrote:
> 
> Hi,
> 
> I created a git repository with these commands:
>  git p4 clone //perforce/path@123456 repo
>  cd repo
>  git p4 rebase
> 
> Some files created before the change 123456 do not exist in git
> history. I do see why,
> but those files were not modified after that change number.
> 
> If I use "git p4 sync --detect-branches" instead of "git p4 rebase",
> the branch contains
> all the files, but not the main trunk, and they appear to be added in
> the first commit of
> the branch.
> 
> To avoid the problem, I must clone with "@all" or with the change number when
> //perforce/path was created, which is significantly longer.
> 
> Regards,
> P. Rouleau

Hi Patrick,

what is your goal here? Do you want to convert the repo to Git or do you
want to use Git to interact with a P4 repo?

I mainly do the former and use the @all switch. You can greatly reduce
the time if your `git-p4` machine is close to the P4 server and if you
reduce the P4 client spec used for the migration.

- Lars

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

[Kaartic Sivaraam]

On Monday 27 November 2017 11:37 AM, Junio C Hamano wrote:

Jeff King  writes:

+using `git --no-optional-locks status` (see linkgit:git[1] for details).


It strikes me just now that `--no-side-effects` might have been a better 
name for the option (of course, iff this avoid all kinds of side 
effects. I'm not sure about the side affects other than index refreshing 
of "git status"). And in case we didn't care about the predictability of 
option names even a little, `--do-what-i-say` might have been a catchy 
alternative ;-)



---
Kaartic

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

[Gennady Kupava]

> 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).

Thanks, missed that, seems my compiler inlined all calls and I didn't
notice the problem.


On 27 November 2017 at 03:25, Junio C Hamano  wrote:
> 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] Makefile: check that tcl/tk is installed

[Junio C Hamano]

Jeff King  writes:

> 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.

OK.

>> 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).

Is that controversial at all?  In the sense that it is addressing a
non-issue (i.e. it is perfectly fine if installed gitk fails at
runtime complaining that it cannot find wish), it might be, but to
me, it appears there isn't any room for controversy there.

But an out-of-box build that fails _is_ a problem worth addressing
to, so I view it primarily as the "how do we do msgfmt (or its
substitute that is good enough for tcl script)" issue.  Perhaps we
can export NO_GETTEXT from the top-level Makefile and have Makefiles
in these two subdirectories to pay attention to it, in addition to
NO_MSGFMT, and build them without i18n support instead of failing,
or something?

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

[Christian Couder]

On Mon, Nov 27, 2017 at 5:35 AM, Jeff King  wrote:
> 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.

What if the user actually don't care about internationalization?

The problem is that inside git-gui, the option to disable msgfmt is
NO_MSGFMT, while in the git repo it is NO_GETTEXT, so if we make this
change in git-gui, we should suggest using NO_MSGFMT to disable
msgfmt. But then the user building git and setting NO_MSGFMT will get
another msgfmt related error later in the git build (as NO_MSGFMT will
have no effect in the git build) and will not understand at all why
there is still a msgfmt error despite setting NO_MSGFMT as the build
suggested.

> 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.

Yeah, I agree. That's why I think it is a good idea if Tcl/Tk is not
installed to ask for either setting NO_TCLTK or installing Tcl/Tk or
setting BYPASS_TCLTK_CHECK.

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

The problem is that we should suggest disabling msgfmt as it is a
valid solution, but as explained above there is an issue related to
NO_MSGFMT in git-gui vs NO_GETTEXT in git.

That's also one of the reason why I don't want to mix the Tcl/Tk issue
and the msgfmt issue. In the end I think we should fix both, but if it
is not possible to fix the simpler Tcl/Tk issue first, it's not even
worth spending time to take a deep look at the msgfmt issue.

>> 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, people have not complained about that and it is not a really bad
situation, but I don't think this is the best practice nor the best we
can do, and I think the situation in this regard is better after my
patch.

For example if someone builds a box that should be used afterwards in
an internal network where this no way to install Tcl/Tk, then users
will be screwed when they will try to run gitk or git-gui. The same
thing can happen if someone installs git just before a long plane trip
with no Internet access. Also the person using git-gui or gitk may not
be the same person as the person installing the box, so the user might
just not have the rights to actually install things.

As I wrote in a previous email, in general it is best to try to fix
issues as soon possible, so it is better to invite people to decide if
they want to install Tcl/Tk at build time rather than at run time.
Yeah, I know that it is not best for packagers and maybe a few other
special cases, but I think setting BYPASS_TCLTK_CHECK should be a good
enough workaround in those cases.

> 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 think any part of my patch should be controversial. I
repeatedly wrote very long messages to show all the possible cases, so
that it is easy to see that we are not worse in any case. And all the
competing suggestions, even the above from Junio either have
significant problems or address a different problem.