* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-12 5:35 UTC (permalink / raw)
To: vi0oss; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <12000496-2191-2915-8a9e-fe7c314c5676@gmail.com>
On Sat, Dec 10, 2016 at 5:41 AM, vi0oss <vi0oss@gmail.com> wrote:
> On 12/08/2016 04:38 AM, vi0oss@gmail.com wrote:
>>
>> Third review: missing && in test fixed.
>>
>
> Shall something more be done about this or just wait until the patch gets
> reviewed and integrated?
I have no further comments and think the most recent version you sent
to the list
is fine. However others are invited to comment as well. :)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Stefan Beller @ 2016-12-12 5:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqk2b7ps08.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 10, 2016 at 10:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Jonathan Nieder mentioned off list that he prefers to see that
>> series rerolled without mutexes if possible. That is possible by
>> creating the questions "struct attr_check" before preloading the
>> index and then using the read only questions in the threaded code,
>> to obtain answers fast; also no need for a mutex.
>
> I do not see how it would work without further splitting the
> attr_stack. I think you made it per check[], but you would further
> split it per <check, thread> before losing the mutex, no?
Well I have not yet looked into it again, so my memories are
rusty, but the <check> is read only, such that the answers only
need to be per thread?
^ permalink raw reply
* Re: [PATCH 1/3] update_unicode.sh: update the uniset repo if it exists
From: Torsten Bögershausen @ 2016-12-12 5:53 UTC (permalink / raw)
To: Beat Bolli, git
In-Reply-To: <1481499265-18361-1-git-send-email-dev+git@drbeat.li>
On 2016-12-12 00:34, Beat Bolli wrote:
> We need to track the new commits in uniset, otherwise their and our code
> get out of sync.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>
> Junio, these go on top of my bb/unicode-9.0 branch, please.
>
> Thanks!
>
> update_unicode.sh | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/update_unicode.sh b/update_unicode.sh
> index 4c1ec8d..9ca7d8b 100755
> --- a/update_unicode.sh
> +++ b/update_unicode.sh
> @@ -14,6 +14,11 @@ fi &&
> http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt &&
> if ! test -d uniset; then
> git clone https://github.com/depp/uniset.git
> + else
> + (
> + cd uniset &&
> + git pull
If upstream has accepted your patches, that's nice.
Minor question, especially to the next commit:
Should we make sure to checkout the exact version, which has been tested?
In this case cb97792880625e24a9f581412d03659091a0e54f
And this is for both a fresh clone and the git pull
needs to be replaced by
git fetch && git checkout cb97792880625e24a9f581412d03659091a0e54f
(Which of course is a shell variable
^ permalink raw reply
* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Junio C Hamano @ 2016-12-12 6:19 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <CAGZ79kbbk4vdW_mbC0riXOf=31V9AQV7zKEh56G+sxjjzAr2-g@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> I do not see how it would work without further splitting the
>> attr_stack. I think you made it per check[], but you would further
>> split it per <check, thread> before losing the mutex, no?
>
> Well I have not yet looked into it again, so my memories are
> rusty, but the <check> is read only, such that the answers only
> need to be per thread?
<check> is read-only, so as long as you populate the singleton
attr's beforehand, they can be shared across threads. <answer>
of course needs an instance per thread, and that is why you have
them typically on stack.
The process of filling <answer> by asking for a set of attrs in
<check> for one <path> goes roughly like:
- make sure attr_stack is set up for <path>, namely, the
info/attributes and .gitattributes files for each leading
directory are parsed.
- go over the attr_stack entries and see what entries match <path>,
and collect the result for <check> in <answer>
Before d90675c151 ("attr: keep attr stack for each check",
2016-11-10), I had only one instance of an attr stack [*1*], but
with that commit, you made it per <check>, which is a good move to
allow us to optimize by keeping only the attributes relevant to
<check> on the attr stack.
But it does not solve the threading issue.
If multiple threads are asking for the same set of attrs
(i.e. running the same codepath using the same <check>) but for
<path>s in different parts of the working tree (e.g. "git checkout"
that is multi-threaded, each thread asking for eol related
attributes and checking out different subdirectories), you'd need
mutex for correct operation at least, but that won't perform well
because you'd end up thrashing the attr stack. You'd need to split
attr stack further and make it per (<check>, thread) tuple and you
no longer need mutex at that point, but not before that.
[footnote]
*1* This is because the attr subsystem originally wasn't designed to
be used from multiple threads at the same time hence it was
sufficient to have a single "currently interested are of the
directory hierarchy".
^ permalink raw reply
* Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
From: David Aguilar @ 2016-12-12 7:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git ML
In-Reply-To: <37d8bc43-9f24-b8e8-cb52-de9cc9b2adde@kdbg.org>
On Sat, Dec 10, 2016 at 09:15:34AM +0100, Johannes Sixt wrote:
> Am 10.12.2016 um 04:21 schrieb David Aguilar:
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> > This patch builds upon da/mergetool-trust-exit-code
> >
> > mergetools/tortoisemerge | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> > index d7ab666a59..9067d8a4e5 100644
> > --- a/mergetools/tortoisemerge
> > +++ b/mergetools/tortoisemerge
> > @@ -1,5 +1,5 @@
> > can_diff () {
> > - return 1
> > + false
> > }
>
> Why is this a simplification?
>
> My concern is that 'false' is not necessarily a shell built-in. Then this is
> actually a pessimization.
The "simplification" is semantic only.
Motivation: if someone reads the implementation of can_diff()
and it says "false" then that communicates intent moreso than
reading "return 1", which a programmer unfamiliar with shell
conventions might misinterpret as boolean "true".
I care less about semantics then I do about making things better
for Windows, so we can forget about these two patches.
--
David
^ permalink raw reply
* [RFC/PATCH] merge: Add '--continue' option as a synonym for 'git commit'
From: Chris Packham @ 2016-12-12 8:34 UTC (permalink / raw)
To: git; +Cc: peff, jacob.keller, gitster, Chris Packham
In-Reply-To: <CAFOYHZAsU_gNb=_K=iMFKFdt60SJ4Wm=Ag5=XMXuQgxNxCqWLA@mail.gmail.com>
Teach 'git merge' the --continue option which allows 'continuing' a
merge by completing it. The traditional way of completing a merge after
resolving conflicts is to use 'git commit'. Now with commands like 'git
rebase' and 'git cherry-pick' having a '--continue' option adding such
an option to 'git merge' presents a consistent UI.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
So here is a quick patch that adds the --continue option. I need to add
some tests (suggestions for where to start are welcome).
Documentation/git-merge.txt | 13 ++++++++++++-
builtin/merge.c | 17 ++++++++++++++++-
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b758d5556..765b0f26e 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -15,6 +15,7 @@ SYNOPSIS
[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
'git merge' <msg> HEAD <commit>...
'git merge' --abort
+'git merge' --continue
DESCRIPTION
-----------
@@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
discouraged: while possible, it may leave you in a state that is hard to
back out of in the case of a conflict.
+The fourth syntax ("`git merge --continue`") can only be run after the
+merge has resulted in conflicts. 'git merge --continue' will take the
+currently staged changes and complete the merge.
OPTIONS
-------
@@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
'git merge --abort' is equivalent to 'git reset --merge' when
`MERGE_HEAD` is present.
+--continue::
+ Take the currently staged changes and complete the merge.
++
+'git merge --continue' is equivalent to 'git commit' when
+`MERGE_HEAD` is present.
+
<commit>...::
Commits, usually other branch heads, to merge into our branch.
Specifying more than one commit will create a merge with
@@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
* Resolve the conflicts. Git will mark the conflicts in
the working tree. Edit the files into shape and
- 'git add' them to the index. Use 'git commit' to seal the deal.
+ 'git add' them to the index. Use 'git merge --continue' to seal the
+ deal.
You can work through the conflict with a number of tools:
diff --git a/builtin/merge.c b/builtin/merge.c
index b65eeaa87..1ce18cbbe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -65,6 +65,7 @@ static int option_renormalize;
static int verbosity;
static int allow_rerere_auto;
static int abort_current_merge;
+static int continue_current_merge;
static int allow_unrelated_histories;
static int show_progress = -1;
static int default_to_upstream = 1;
@@ -223,6 +224,8 @@ static struct option builtin_merge_options[] = {
OPT__VERBOSITY(&verbosity),
OPT_BOOL(0, "abort", &abort_current_merge,
N_("abort the current in-progress merge")),
+ OPT_BOOL(0, "continue", &continue_current_merge,
+ N_("continue the current in-progress merge")),
OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
N_("allow merging unrelated histories")),
OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
@@ -739,7 +742,7 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
if (err_msg)
error("%s", err_msg);
fprintf(stderr,
- _("Not committing merge; use 'git commit' to complete the merge.\n"));
+ _("Not committing merge; use 'git merge --continue' to complete the merge.\n"));
write_merge_state(remoteheads);
exit(1);
}
@@ -1166,6 +1169,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
goto done;
}
+ if (continue_current_merge) {
+ int nargc = 1;
+ const char *nargv[] = {"commit", NULL};
+
+ if (!file_exists(git_path_merge_head()))
+ die(_("There is no merge in progress (MERGE_HEAD missing)."));
+
+ /* Invoke 'git commit' */
+ ret = cmd_commit(nargc, nargv, prefix);
+ goto done;
+ }
+
if (read_cache_unmerged())
die_resolve_conflict("merge");
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/3] update_unicode.sh: update the uniset repo if it exists
From: Beat Bolli @ 2016-12-12 8:54 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <64bc846c-0304-dd7b-73bf-a6c3a4135381@web.de>
On 2016-12-12 06:53, Torsten Bögershausen wrote:
> On 2016-12-12 00:34, Beat Bolli wrote:
>> We need to track the new commits in uniset, otherwise their and our
>> code
>> get out of sync.
>>
>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> ---
>>
>> Junio, these go on top of my bb/unicode-9.0 branch, please.
>>
>> Thanks!
>>
>> update_unicode.sh | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/update_unicode.sh b/update_unicode.sh
>> index 4c1ec8d..9ca7d8b 100755
>> --- a/update_unicode.sh
>> +++ b/update_unicode.sh
>> @@ -14,6 +14,11 @@ fi &&
>> http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt &&
>> if ! test -d uniset; then
>> git clone https://github.com/depp/uniset.git
>> + else
>> + (
>> + cd uniset &&
>> + git pull
> If upstream has accepted your patches, that's nice.
>
> Minor question, especially to the next commit:
> Should we make sure to checkout the exact version, which has been
> tested?
> In this case cb97792880625e24a9f581412d03659091a0e54f
>
> And this is for both a fresh clone and the git pull
> needs to be replaced by
> git fetch && git checkout cb97792880625e24a9f581412d03659091a0e54f
>
>
> (Which of course is a shell variable)
I was actually wondering what the policy was for adding submodules to
the Git repo,
but then decided against it. Another option would be to fork uniset on
GitHub and
just let it stay on a working commit.
Junio, what's your stance on this?
Beat
^ permalink raw reply
* Re: [RFC/PATCH] merge: Add '--continue' option as a synonym for 'git commit'
From: Markus Hitter @ 2016-12-12 9:02 UTC (permalink / raw)
To: Chris Packham, git; +Cc: peff, jacob.keller, gitster
In-Reply-To: <20161212083413.7334-1-judge.packham@gmail.com>
Am 12.12.2016 um 09:34 schrieb Chris Packham:
> Teach 'git merge' the --continue option which allows 'continuing' a
> merge by completing it. The traditional way of completing a merge after
> resolving conflicts is to use 'git commit'. Now with commands like 'git
> rebase' and 'git cherry-pick' having a '--continue' option adding such
> an option to 'git merge' presents a consistent UI.
Like.
While Junio is entirely right that this is redundant, the inner workings of Git are just voodoo for a (guessed) 95% of users out there, so a consistent UI is important.
> DESCRIPTION
> -----------
> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
> discouraged: while possible, it may leave you in a state that is hard to
> back out of in the case of a conflict.
>
> +The fourth syntax ("`git merge --continue`") can only be run after the
> +merge has resulted in conflicts. 'git merge --continue' will take the
> +currently staged changes and complete the merge.
I think this should mention the equivalence to 'git commit'.
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
^ permalink raw reply
* Re: Proposal for an increased `gitk` cohesion with `git stash`.
From: Paul Mackerras @ 2016-12-12 9:36 UTC (permalink / raw)
To: Uxío Prego; +Cc: git
In-Reply-To: <CANidDKZRisodpQgqyYaG_tCi0+EyxYv+t8+Entp0joMSetd3oA@mail.gmail.com>
Hi Uxio,
On Thu, Sep 08, 2016 at 03:41:29PM +0200, Uxío Prego wrote:
> Hello, please forgive me for not introducing me.
>
> +-----------+
> |Description|
> +-----------+
>
> Patch for showing all stashes in `gitk`.
>
> +-----------+
> |The problem|
> +-----------+
>
> Being `gitk` one of the best tools for navigating and understanding graphs
> of git repo revs, I got used to have it for home use, some years ago, soon
> for office use too.
>
> At some point I got used to invoke always `gitk --all` in order to keep
> tracking of tags when using the program for building, when possible, stable
> versions of any GNU/Linux software I would want to use.
>
> It seems `gitk --all` uses `git show-ref` for showing remotes information.
> A side effect of this is that the most recent stash gets shown in `gitk
> --all`. After learning what the stash was, I got used to it.
>
> Later, when at jobs, I got used to have a stash on all working branch tips.
> So I often would have several stashes in `git stash list` but only the last
> one in `gitk --all`. That annoyed me for about a year, finally I got
> working in `gitk` to show a stash status more similar to what `git stash
> list` shows.
>
> +-----------+
> |The feature|
> +-----------+
>
> Show all stashes in `gitk` instead of only the last one.
This seems like a good feature, although I don't use stashes myself.
> +------------------+
> |Why it's desirable|
> +------------------+
>
> In order to have better visual control when working on repos that have
> several active branches and there are needed quick context changes between
> them with the features that `git stash [apply [STASHNAME]| list | pop
> [STASHNAME]| push | save | show [[-p] STASHNAME]]`.
>
> In order to have a better cohesion between the mentioned features and `gitk
> --all`.
>
> +------------------------+
> |Caveats and side effects|
> +------------------------+
>
> With this patch several side effects happen:
>
> * Stashes are shown even invoking `gitk`, not only when running `gitk
> --all`. If this is a problem, I can keep working in the patch to avoid this.
>
> * The most recent stash, which was previously shown as 'stash' because its
> corresponding `git show-ref` output line reads 'refs/stash', gets shown as
> 'stash@{0}'. Not removing lines with 'stash' when calling `git show-ref`
> gets it shown both as 'stash' as usual and as 'stash@{0}'.
>
> +--------------------------+
> |Non-obvious design choices|
> +--------------------------+
>
> There are many improvable things consequence of never having edited
> anything Tcl before this. Besides, its working for me as a proof of
> concept, both in Debian 7 Wheezy and 8 Jessie.
>
> The origin document of the following diff is `gitk` as it ships in Debian 8
> Jessie. I have not tried yet but if required I would be willing to rework
> it for the repo master.
A patch against the latest version in my git repo at
git://ozlabs.org/~paulus/gitk would be better.
> `gitk-stash-list-ids.sh` is a shell script that performs `git stash list
> --pretty=format:"%H"`, i.e. show rev hashes for all stashes in the fashion
> that `git rev-list --all` does its default output. I did this because I
> could not work out a better pure Tcl solution.
Hmmm, I don't want gitk to have to depend on an external script.
It should be possible to make Tcl execute the git command directly,
though (see below).
> +--------------------+
> |Unified diff follows|
> +--------------------+
>
> 0:10:1473338052:uprego@uxio:~$ diff -u /usr/bin/gitk-deb8-vanilla
> /usr/bin/gitk-deb8-multistash
> --- /usr/bin/gitk-deb8-vanilla 2016-08-29 10:07:06.507344629 +0200
> +++ /usr/bin/gitk-deb8-multistash 2016-09-08 14:36:35.382476634 +0200
> @@ -401,6 +401,10 @@
>
> if {$vcanopt($view)} {
> set revs [parseviewrevs $view $vrevs($view)]
> + set stashesfd [open [concat | gitk-stash-list-ids.sh] r]
set stashesfd [open [list | git stash list {--pretty=format:%H}] r]
> + while {[gets $stashesfd stashline] >= 0} {
> + set revs [lappend revs $stashline]
> + }
Could this ever take a long time? The UI is unresponsive while we're
in this loop. If this is always quick (even on large repos), OK. If
it could take a long time then we'll need a file event handler.
> if {$revs eq {}} {
> return 0
> }
> @@ -1778,7 +1782,7 @@
> foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
> catch {unset $v}
> }
> - set refd [open [list | git show-ref -d] r]
> + set refd [open [list | git show-ref -d | grep -v stash] r]
If I had a branch called "dont-use-a-stash-for-this", would it get
filtered out by this grep? It seems like it would, and we don't want
it to. So the filtering needs to be more exact here.
> while {[gets $refd line] >= 0} {
> if {[string index $line 40] ne " "} continue
> set id [string range $line 0 39]
> @@ -1811,6 +1815,16 @@
> }
> }
> catch {close $refd}
> + set stashesidsrefsd [open [list | gitk-stash-list-ids-refs.sh] r]
set stashesidsrefsd [open [list | \
git stash list {--pretty=format:%H %gD}] r]
Paul.
^ permalink raw reply
* Re: [PATCH] gitk: Fix Japanese translation for "marked commit"
From: Paul Mackerras @ 2016-12-12 9:37 UTC (permalink / raw)
To: Satoshi Yasushima; +Cc: git
In-Reply-To: <20161024153510.9212-1-s.yasushima@gmail.com>
On Tue, Oct 25, 2016 at 12:35:10AM +0900, Satoshi Yasushima wrote:
> Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
> ---
> po/ja.po | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
Applied, thanks,
Paul.
^ permalink raw reply
* Re: [RFC/PATCH] merge: Add '--continue' option as a synonym for 'git commit'
From: Jeff King @ 2016-12-12 9:40 UTC (permalink / raw)
To: Chris Packham; +Cc: git, jacob.keller, gitster
In-Reply-To: <20161212083413.7334-1-judge.packham@gmail.com>
On Mon, Dec 12, 2016 at 09:34:13PM +1300, Chris Packham wrote:
> Teach 'git merge' the --continue option which allows 'continuing' a
> merge by completing it. The traditional way of completing a merge after
> resolving conflicts is to use 'git commit'. Now with commands like 'git
> rebase' and 'git cherry-pick' having a '--continue' option adding such
> an option to 'git merge' presents a consistent UI.
>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
> So here is a quick patch that adds the --continue option. I need to add
> some tests (suggestions for where to start are welcome).
I'm not sure if there's much to test besides concluding a successful
merge, and possibly some error cases where --continue should complain.
Probably that could go at the end of t7600.
> @@ -1166,6 +1169,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> goto done;
> }
>
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (!file_exists(git_path_merge_head()))
> + die(_("There is no merge in progress (MERGE_HEAD missing)."));
> +
> + /* Invoke 'git commit' */
> + ret = cmd_commit(nargc, nargv, prefix);
> + goto done;
> + }
> +
I know this block is just adapted from the "--abort" one above, but
should both of these complain when other arguments are given? I can't
imagine what the user might mean with "git merge --no-commit
--continue", but probably it should be an error. :)
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] gitk: memory consumption improvements
From: Paul Mackerras @ 2016-12-12 9:51 UTC (permalink / raw)
To: Markus Hitter; +Cc: git@vger.kernel.org
In-Reply-To: <de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de>
On Mon, Nov 07, 2016 at 07:54:28PM +0100, Markus Hitter wrote:
>
> List, Paul,
>
> after searching for a while on why Gitk sometimes consumes exorbitant amounts of memory I found a pair of minor issues and also a big one: the text widget comes with an unlimited undo manager, which is turned on be default. Considering that each line is inserted seperately, this piles up a huuuge undo stack ... for a read-only text widget. Simply turning off this undo manager saves about 95% of memory when viewing large commits (with tens of thousands of diff lines).
>
> 3 patches are about to follow:
>
> - turn off the undo manager,
>
> - forget already closed file descriptors and
>
> - forget the 'commitinfo' array on a reload to enforce reloading it.
>
> I hope this finds you appreciation.
Thanks for the good work in tracking this down and making the patches.
I have applied the series. Apologies for slow response (life has been
extremely busy for me this year).
Paul.
^ permalink raw reply
* Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows
From: Johannes Schindelin @ 2016-12-12 9:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pranit Bauva
In-Reply-To: <xmqqmvg2nyo6.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Sun, 11 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > I finally got a chance to debug the problems with the ever-timing-out
> > test runs of `pu` on Windows. Turns out that pb/bisect uncovered a
> > really old, really bad bug: on Windows, isatty() does not do what Git
> > expects, at least not completely: it detects interactive terminals *and
> > character devices*.
>
> Sounds as if somebody who did Windows at Microsoft had a good sense
> of humor to mimick the misnamed ENOTTY gotcha ;-)
Hehe...
> This is a great find, and a very impactful fix, as redirecting from
> /dev/null is how we try to force a "go interactive if talking to
> tty" program to realize that it is not talking to a tty.
Can we fast-track this to maint?
I will definitely ship this fix in the next Git for Windows version, but
still, it would be nice to have this in git.git as soon as possible.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-12-12 10:42 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, Junio C Hamano, Jakub Narębski
In-Reply-To: <CA+P7+xquordVY19dypqNcAuQqoRbFmHhzb0w+HXCaJmm_Ex7zQ@mail.gmail.com>
On Thu, Dec 8, 2016 at 5:31 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index f4ad297..c72baeb 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -92,13 +92,14 @@ refname::
>> The name of the ref (the part after $GIT_DIR/).
>> For a non-ambiguous short name of the ref append `:short`.
>> The option core.warnAmbiguousRefs is used to select the strict
>> - abbreviation mode. If `strip=<N>` is appended, strips `<N>`
>> - slash-separated path components from the front of the refname
>> - (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>> - `<N>` must be a positive integer. If a displayed ref has fewer
>> - components than `<N>`, the command aborts with an error. For the base
>> - directory of the ref (i.e. foo in refs/foo/bar/boz) append
>> - `:base`. For the entire directory path append `:dir`.
>> + abbreviation mode. If `lstrip=<N>` or `rstrip=<N>` option can
>
> Grammar here, drop the If before `lstrip since you're referring to
> multiples and you say "x can be appended to y" rather than "if x is
> added, do y"
>
Will do. Thanks.
>> + be appended to strip `<N>` slash-separated path components
>> + from or end of the refname respectively (e.g.,
>> + `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
>> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`). if
>> + `<N>` is a negative number, then only `<N>` path components
>> + are left behind. If a displayed ref has fewer components than
>> + `<N>`, the command aborts with an error.
>>
>
> Would it make more sense to not die and instead just return the empty
> string? On the one hand, if we die() it's obvious that you tried to
> strip too many components. But on the other hand, it's also somewhat
> annoying to have the whole command fail because we happen upon a
> single ref that has fewer components?
>
> So, for positive numbers, we simply strip what we can, which may
> result in the empty string, and for negative numbers, we keep up to
> what we said, while potentially keeping the entire string. I feel
> that's a better alternative than a die() in the middle of a ref
> filter..
>
> What are other people's thoughts on this?
I am _for_ this. Even I think it'd be better to return an empty string rather
than just die in the middle.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-12-12 10:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller, Jakub Narębski
In-Reply-To: <xmqqpol2rn0z.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 9, 2016 at 5:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> Will replace, with the attached stylistic fixes squashed in for
> minor issues that were spotted by my mechanical pre-acceptance
> filter.
>
Thanks for this. Will add it to my local branch too if there's a need
for a re-roll.
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Johannes Schindelin @ 2016-12-12 11:03 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: git
In-Reply-To: <20161211110208.642unp7c2i653sav@ikki.ethgen.ch>
Hi Klaus,
On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > There are too many systems out there that use a backslash in path names. I
> > > > don't think it is wise to use it also as the quoting character.
> > > Well, the minority, I believe. And only the minority where the command
> > > line git is used anywhere.
> >
> > Please provide evidence for such bold assumptions.
>
> How is it "bold" to see that the majority of widows users does not use
> or even like command line tools.
First of all, it is "Windows users", not "widows users", unless, of
course, you want to discuss things that are completely inappropriate for
this list.
Second, you still did not back up your claim with anything resembling
evidence, instead just reiterating your beliefs. That is not good enough.
Third, my experience contradicts your beliefs rather violently.
So let's try some evidence for a change: the 64-bit installer of Git for
Windows v2.10.2 was downloaded over 1.25 million times. That is not a
negligible number. If you want to go back to v2.8.1, it is even 3.8
million downloads. That is a tall number to call a minority.
Now let's look at your claim that Windows users do not use the
command-line. The mere existence of posh-git (Powershell bindings for Git)
is already a contradiction to that claim.
Even if that was not enough, the Git for Windows bug tracker is full of
reports of users who clearly use the command-line.
And there is more evidence: When comparing the download numbers of the
different Git for Windows versions, one thing really sticks out: those
versions were downloaded the most (by a factor of more than 2x over the
other versions) which were made available through Visual Studio's
"Download command-line Git tools" feature, e.g. v2.8.1 and v2.9.0. That is
a rather strong indicator that users wanted to use the command-line.
Fourth, even if Windows users were the minority, and even if Windows users
were not using the command-line, which are claims soundly refuted by the
evidence I presented above, the fact alone that you are talking about
putting a group of people at a disadvantage based merely on your belief
that they are in a minority should not inform us, the Git developers, on
any kind of policy decision.
We will not intentionally break Git usage, or make Git usage hard, for
a specific group of Git users, unless there are technical reasons to
do that. Demographic reasons do not count.
For example, we will not make Git hard to use for female programmers,
on the grounds that they currently constitute a minority.
> I know companies where the "developers" doesn't even know of the
> existent of a git command line use. They look with owe when they see
> that I use a shell to use git.
I must have spoken to hundreds of Git for Windows users, and must have
been in communication with many more via email or bug tracker, and I
cannot recall a single one who used Git without using the command-line.
Note: I do not count my personal experience here as evidence, but the
numbers alone are a strong indicator to me that your argument has a pretty
weak foundation.
Ciao,
Johannes
P.S.: Maybe reply-to-all in the future; it is the custom on this here
mailing list.
^ permalink raw reply
* [PATCH] git-svn: allow "0" in SVN path components
From: Eric Wong @ 2016-12-12 11:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Blindly checking a path component for falsiness is unwise, as
"0" is false to Perl, but a valid pathname component for SVN
(or any filesystem).
Found via random code reading.
Signed-off-by: Eric Wong <e@80x24.org>
---
Junio: this bugfix should go to "maint".
Will push along with a doc fix for Juergen.
perl/Git/SVN/Ra.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index e764696801..56ad9870bc 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -606,7 +606,7 @@ sub minimize_url {
my $latest = $ra->get_latest_revnum;
$ra->get_log("", $latest, 0, 1, 0, 1, sub {});
};
- } while ($@ && ($c = shift @components));
+ } while ($@ && defined($c = shift @components));
return canonicalize_url($url);
}
--
EW
^ permalink raw reply related
* [PULL] minor git-svn updates (probably for 2.11.x)
From: Eric Wong @ 2016-12-12 11:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The following changes since commit 8d7a455ed52e2a96debc080dfc011b6bb00db5d2:
Start post 2.11 cycle (2016-12-05 11:31:47 -0800)
are available in the git repository at:
git://bogomips.org/git-svn.git master
for you to fetch changes up to 1b7edec78b754a1e901c164a4bf4e94bff96ed7b:
git-svn: document useLogAuthor and addAuthorFrom config keys (2016-12-12 11:09:29 +0000)
----------------------------------------------------------------
Eric Wong (2):
git-svn: allow "0" in SVN path components
git-svn: document useLogAuthor and addAuthorFrom config keys
Documentation/git-svn.txt | 8 +++++++-
perl/Git/SVN/Ra.pm | 2 +-
2 files changed, 8 insertions(+), 2 deletions(-)
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-12 11:20 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161209140345.76ybodldmg2lxgbn@sigill.intra.peff.net>
On Fri, Dec 9, 2016 at 7:33 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Dec 07, 2016 at 09:06:26PM +0530, Karthik Nayak wrote:
>
>> +const char *quote_literal_for_format(const char *s)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>>
>> + strbuf_reset(&buf);
>> + while (*s) {
>> + const char *ep = strchrnul(s, '%');
>> + if (s < ep)
>> + strbuf_add(&buf, s, ep - s);
>> + if (*ep == '%') {
>> + strbuf_addstr(&buf, "%%");
>> + s = ep + 1;
>> + } else {
>> + s = ep;
>> + }
>> }
>> + return buf.buf;
>> }
>
> You should use strbuf_detach() to return the buffer from a strbuf.
> Otherwise it is undefined whether the pointer is allocated or not (and
> whether it needs to be freed).
>
> In this case, if "s" is empty, buf.buf would point to a string literal,
> but otherwise to allocated memory. strbuf_detach() normalizes that.
>
> But...
>
>> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
>
> This caller never stores the return value, and it ends up leaking. So I
> wonder if you wanted "static struct strbuf" in the first place (and that
> would explain the strbuf_reset() in your function).
>
> -Peff
Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
suggestion.
strbuf_detach() is also a better way to go.
Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
From: Philip Oakley @ 2016-12-12 11:44 UTC (permalink / raw)
To: David Aguilar, Johannes Sixt; +Cc: Junio C Hamano, Git ML
In-Reply-To: <20161212071646.5bqnnjpfnmnj6fm4@gmail.com>
From: "David Aguilar" <davvid@gmail.com>
> On Sat, Dec 10, 2016 at 09:15:34AM +0100, Johannes Sixt wrote:
>> Am 10.12.2016 um 04:21 schrieb David Aguilar:
>> > Signed-off-by: David Aguilar <davvid@gmail.com>
>> > ---
>> > This patch builds upon da/mergetool-trust-exit-code
>> >
>> > mergetools/tortoisemerge | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
>> > index d7ab666a59..9067d8a4e5 100644
>> > --- a/mergetools/tortoisemerge
>> > +++ b/mergetools/tortoisemerge
>> > @@ -1,5 +1,5 @@
>> > can_diff () {
>> > - return 1
>> > + false
>> > }
>>
>> Why is this a simplification?
>>
>> My concern is that 'false' is not necessarily a shell built-in. Then this
>> is
>> actually a pessimization.
>
> The "simplification" is semantic only.
>
> Motivation: if someone reads the implementation of can_diff()
> and it says "false" then that communicates intent moreso than
> reading "return 1", which a programmer unfamiliar with shell
> conventions might misinterpret as boolean "true".
>
Is this a case where a short comment would be informative?
+ return 1 /* shell: false */
> I care less about semantics then I do about making things better
> for Windows, so we can forget about these two patches.
> --
> David
>
--
Philip
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-12 11:50 UTC (permalink / raw)
To: git
In-Reply-To: <alpine.DEB.2.20.1612121133220.23160@virtualbox>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi,
Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > There are too many systems out there that use a backslash in path names. I
> > > > > don't think it is wise to use it also as the quoting character.
> > > > Well, the minority, I believe. And only the minority where the command
> > > > line git is used anywhere.
> > > Please provide evidence for such bold assumptions.
> > How is it "bold" to see that the majority of widows users does not use
> > or even like command line tools.
>
> First of all, it is "Windows users", not "widows users", unless, of
> course, you want to discuss things that are completely inappropriate for
> this list.
Sorry to have a typo in my response.
> Second, you still did not back up your claim with anything resembling
> evidence, instead just reiterating your beliefs. That is not good enough.
Well, my evidence is what I seen with many windows users in the past. I
have no link or something like that. I just shared that observation.
> Third, my experience contradicts your beliefs rather violently.
So we have two different observations. Good. Have no problem with that.
[...]
> Now let's look at your claim that Windows users do not use the
> command-line. The mere existence of posh-git (Powershell bindings for Git)
> is already a contradiction to that claim.
Is that the same git tools or is it a separate implementation?
[Proof of many downloads and other]
Proofs accepted. They do not match my observations but ok.
> Fourth, even if Windows users were the minority, and even if Windows users
> were not using the command-line, which are claims soundly refuted by the
> evidence I presented above, the fact alone that you are talking about
> putting a group of people at a disadvantage based merely on your belief
> that they are in a minority should not inform us, the Git developers, on
> any kind of policy decision.
>
> We will not intentionally break Git usage, or make Git usage hard, for
> a specific group of Git users, unless there are technical reasons to
> do that. Demographic reasons do not count.
Sorry, but I was posting my answer to the comment of preferring not to
allow ":" in prefer of using "\" for quoting. I never wanted to attack
you. It was just a response... And I do not remember attacking you
personally to style me "bold".
Currently the source of that all was, that push to pathes with ":" did
break in the current version. I did not ask for implementing something
instead of something different. I just ask for fixing of a regression.
Moreover, as you might know, windows users are more affected by that bug
as they have a colon in every absolute path. Nevertheless I had that
problem on linux but with a map to a windows share.
For more, I have to agree with Philip, that this is a bit off topic. So
please stop taking single words on the personal level. Please stay on
the technical level. My english is to bad to go to personal level
discussions. And I don't want to spend time to personal or (as you
tried) feminism discussions.
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhOjvwACgkQpnwKsYAZ
9qyb2wwAuUFTRrOQcJ6mDLx5bHPNrQy6e4ify4qWcfcp3VJhOLpA56CdJwwA2bcl
gcllUwOn+SSjjYaRmuFQ6aY0/oKDEtF98Zh01pXLnAgY+Dabg9gfBCMWyWv8LxUf
NWHADPpSkY2eongvm6aXOGTqNvU73wriuQKM6BwG49bJUpdR1q8Fe+DjtufrS8eW
ALeGoEFrbm0aIbd121AvWw53hCz3j9ssDpGdFefkhjeJnRcSSDmzTID5KcuWME9P
L67BjGJb3swYIpw3Sdg7LjWnMK0o9GpzfZVNuJFMsRHBQUaWAQvVG79AHe/5QI9w
pq0K5C6frllP5CgcoD2/H+8F8sMD7BrhyN1MxFRdaa4eCEh/hFxZV2nIfbZnx0SS
5/SNMKcCdly+yodZCgohU4uJuqDBkRIEbr6+OCffsupQMCYkh/Ew/RRa8tMN26bN
45/ferqMK1KEenpllXGNHi3/a9dT5CaqEvLjErdatChhQR6i7QbA5B3KhXKRSsRz
YPbYc7rc
=e8K5
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Jeff King @ 2016-12-12 12:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <CAOLa=ZSPDLwziGEvyixebAkS2M1JMYidQNHfDbnmYarFCjn80A@mail.gmail.com>
On Mon, Dec 12, 2016 at 04:50:20PM +0530, Karthik Nayak wrote:
> > This caller never stores the return value, and it ends up leaking. So I
> > wonder if you wanted "static struct strbuf" in the first place (and that
> > would explain the strbuf_reset() in your function).
>
> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
> suggestion.
>
> strbuf_detach() is also a better way to go.
One of the other, though. If it's static, then you _don't_ want to
detach.
-Peff
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Johannes Schindelin @ 2016-12-12 14:05 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: git
In-Reply-To: <20161212115030.qx2y276bxnzbtxkj@ikki.ethgen.ch>
Hi Klaus,
you probably missed the note about reply-to-all being customary on this
list. The reason is that this list is high-volume, and open to
non-subscribers. You may want to get into the habit of hitting
reply-to-all when responding to mails from this list, in case that you
plan to continue communicating on this forum.
On Mon, 12 Dec 2016, Klaus Ethgen wrote:
> Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> > On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > > There are too many systems out there that use a backslash in path names. I
> > > > > > don't think it is wise to use it also as the quoting character.
> > > > > Well, the minority, I believe. And only the minority where the command
> > > > > line git is used anywhere.
> > > > Please provide evidence for such bold assumptions.
> > > How is it "bold" to see that the majority of widows users does not use
> > > or even like command line tools.
> >
> > Second, you still did not back up your claim with anything resembling
> > evidence, instead just reiterating your beliefs. That is not good enough.
>
> Well, my evidence is what I seen with many windows users in the past. I
> have no link or something like that. I just shared that observation.
Thank you for confirming that you just shared a personal observation and
did not plan to back that up by scientific evidence.
In light of that, let's continue according to Johannes Sixt's insightful
suggestions.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-12 16:29 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161212121548.aj2ptnmrqt67anlc@sigill.intra.peff.net>
On Mon, Dec 12, 2016 at 5:45 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 12, 2016 at 04:50:20PM +0530, Karthik Nayak wrote:
>
>> > This caller never stores the return value, and it ends up leaking. So I
>> > wonder if you wanted "static struct strbuf" in the first place (and that
>> > would explain the strbuf_reset() in your function).
>>
>> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
>> suggestion.
>>
>> strbuf_detach() is also a better way to go.
>
> One of the other, though. If it's static, then you _don't_ want to
> detach.
>
Wait. Why not? since it gets added to the strbuf's within
build_format() and that
value is not needed again, why do we need to re-allocate? we can use the same
variable again (i.e by keeping it as static).
I'm sorry, but I didn't get why these two should be mutually exclusive?
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Jeff King @ 2016-12-12 16:40 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <CAOLa=ZT1KN_iw7JzB78hPb-LrY_yaDZk0D+TqY2W9hCOftzumA@mail.gmail.com>
On Mon, Dec 12, 2016 at 09:59:49PM +0530, Karthik Nayak wrote:
> >> > This caller never stores the return value, and it ends up leaking. So I
> >> > wonder if you wanted "static struct strbuf" in the first place (and that
> >> > would explain the strbuf_reset() in your function).
> >>
> >> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
> >> suggestion.
> >>
> >> strbuf_detach() is also a better way to go.
> >
> > One of the other, though. If it's static, then you _don't_ want to
> > detach.
> >
>
> Wait. Why not? since it gets added to the strbuf's within
> build_format() and that
> value is not needed again, why do we need to re-allocate? we can use the same
> variable again (i.e by keeping it as static).
>
> I'm sorry, but I didn't get why these two should be mutually exclusive?
What is the memory ownership convention for the return value from the
function?
If the caller should own the memory, then you want to do this:
char *foo(...)
{
struct strbuf buf = STRBUF_INIT;
... fill up buf ...
return strbuf_detach(&buf, NULL);
}
The "detach" disconnects the memory from the strbuf (which is going out
of scope anyway), and the only pointer left to it is in the caller. It's
important to use "detach" here and not just return the pointer, because
that ensures that the returned value is always allocated memory (and
never strbuf_slopbuf).
If the caller should not own the memory, then the function retains
ownership. And you want something like this:
char *foo(...)
{
static struct strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
... fill up buf ...
return buf.buf;
}
The same buffer is reused over and over. The "reset" call clears any
leftover bits from the last invocation, and you must _not_ call
strbuf_detach() in the return, as it disconnects the memory from the
strbuf (and so next time you'd end up allocating again, and each return
value becomes a memory leak).
Does that make sense?
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox