* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 20:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <f14ee492-8297-c8ec-f80f-f8f24caf91e1@kdbg.org>
On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 20:23 schrieb Stefan Beller:
>>
>> In a reroll I'll drop this patch and instead introduce
>> a function `char *get_child_command_line(struct child_process*);`, which
>> a caller can call before calling finish_command and then use the
>> resulting string
>> to assemble an error message without lego.
>
>
> That sounds a lot better, thank you. Note though, that the function would
> have to be called before start_command(), because when it fails, it would be
> too late.
Yes, the pattern to use it would be
// first assemble the child process struct with conditions
char *cmdline = get_child_command_line(&child)
if (start_command(&child))
// use cmdline here for error reporting.
>
> I have to ask, though: Is it so much necessary to report the exact command
> line in an error message?
Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
die("Could not run 'git status --porcelain -uall \
--ignore-submodules=none' in submodule %s", path);
There are 2 things:
1) The error message is not very informative, as your question hints at.
I consider changing it to add more meaning. I think the end user
would also be interested in "Why did we run this command?".
2) We really want to report the correct command line here.
Currently that is the case, but if you look at the prior patch that extends
the arguments conditionally (and then also uses the same condition
to format the error message... that hints at hard to maintain error
messages.)
So the new proposed function really only addresses 2) here.
> If someone is interested in which command failed,
> it would be "sufficient" to run the command under GIT_TRACE=2.
>
Yes, while at it, I may just move up the error reporting to the builtin
command giving a higher level message.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johan
In-Reply-To: <xmqqk2aujsyb.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > In typical uses of fast-import, trees are inherited from a parent
> > commit. In that case, the tree_entry for the branch looks like:
> > ...
> > +# Create another notes tree from the one above
> > +cat >>input <<INPUT_END
> > +...
> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
>
> There is a trailing SP that cannot be seen by anybody.
>
> Don't do this. It makes it very easy to miss what is going on and
> wastes reviewers' time.
>
> Protect it by doing something like:
>
> sed -e 's/Z$//' >>input <<INPUT_END
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
How about
EMPTY=
...
M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
?
Mike
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 20:12 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <CAGZ79kYNKWfnEXWJfyRUutFyaQiRD9qW--LkK4Nbwdf7FtdPQA@mail.gmail.com>
Am 20.12.2016 um 20:23 schrieb Stefan Beller:
> In a reroll I'll drop this patch and instead introduce
> a function `char *get_child_command_line(struct child_process*);`, which
> a caller can call before calling finish_command and then use the
> resulting string
> to assemble an error message without lego.
That sounds a lot better, thank you. Note though, that the function
would have to be called before start_command(), because when it fails,
it would be too late.
I have to ask, though: Is it so much necessary to report the exact
command line in an error message? If someone is interested in which
command failed, it would be "sufficient" to run the command under
GIT_TRACE=2.
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
From: Junio C Hamano @ 2016-12-20 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, josharian
In-Reply-To: <20161220094836.4131-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> We could rely on atexit() to clean up everything, but let's be
> explicit when we can. And it's good anyway because the function is
> called the second time in the same process, we're in trouble.
>
> This function should not affect the successful case because after
> commit_lock_file() is called, rollback_lock_file() becomes no-op.
Not really. At the point of the first "goto out" in this function,
lock is merely an uninitialized pointer.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> config.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/config.c b/config.c
> index 505e0d0..e02def4 100644
> --- a/config.c
> +++ b/config.c
> @@ -2483,6 +2483,7 @@ int git_config_rename_section_in_file(const char *config_filename,
> ret = error_errno("could not write config file %s",
> config_filename);
> out:
> + rollback_lock_file(lock);
> free(filename_buf);
> return ret;
> }
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 19:34 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161219021212.15978-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> +# Create another notes tree from the one above
> +cat >>input <<INPUT_END
> +...
> +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
There is a trailing SP that cannot be seen by anybody.
Don't do this. It makes it very easy to miss what is going on and
wastes reviewers' time.
Protect it by doing something like:
sed -e 's/Z$//' >>input <<INPUT_END
...
M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] push only submodules
From: Junio C Hamano @ 2016-12-20 19:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> This series teaches 'git push' to be able to only push submodules
> while leaving a superproject unpushed.
It somehow feels a bit strange to single out the top-level as
special like this one (iow, shouldn't it be equally as easy to push
out the superproject and two submodules leaving one submodule alone,
as to push out these three submodules leaving the superproject
alone?), but I will queue these for now with tweaks to address a few
minor points.
* What is done with 1/3 I do not think is "refactor".
* "perhaps a code review tool" -> "perhaps a tool like Gerrit code
review" as suggested by Stefan.
* Swap the order expected_pub and expected_submodule are generated
to match the order they are used for final verification in the
test.
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 19:23 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org>
On Tue, Dec 20, 2016 at 10:33 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>>
>> +static void report_and_die(struct child_process *cmd, const char *action)
>> +{
>> + int i;
>> + struct strbuf err = STRBUF_INIT;
>> + if (cmd->git_cmd)
>> + strbuf_addstr(&err, "git ");
>> + for (i = 0; cmd->argv[i]; )
>> + strbuf_addf(&err, "'%s'", cmd->argv[i]);
>
>
> Take note that cmd is accessed here.
>
>> + die(_("could not %s %s"), action, err.buf);
>
>
> Should lego sentences not be avoided? They are not exactly translator
> friendly.
>
> Given that a lot of effort is spent elsewhere to actually *avoid* dying in
> library code, this new die() is not very welcome, I must say.
I agree on the sentiment. In a reroll I'll drop this patch and instead introduce
a function `char *get_child_command_line(struct child_process*);`, which
a caller can call before calling finish_command and then use the
resulting string
to assemble an error message without lego.
Thanks for the thorough review!
Stefan
^ permalink raw reply
* [GIT GUI/PATCH/RFC] git gui: get current theme in 8.5-backwards-compatible way
From: Pete Harlan @ 2016-12-20 19:20 UTC (permalink / raw)
To: Pat Thoyts; +Cc: git
Later Tcl 8.5 versions learned to return the current theme by omitting
the final argument to "[ttk::style theme use]", but this throws an
error on earlier 8.5 versions (e.g., 8.5.7).
InitTheme works around this by catching the error and reading
::ttk::currentTheme instead.
A call to "[ttk::style theme use]" was added to ttext in 30508bc
("Amend tab ordering and text widget border and highlighting.",
2016-10-02). Break out InitTheme's workaround into its own
get_current_theme proc and use it in both places.
Signed-off-by: Pete Harlan <pgit@tento.net>
---
Note: Applies to the upstream git-gui repo, http://repo.or.cz/git-gui .
To apply to Git itself, apply in the git-gui directory.
Issue entered Git in v2.10.1-537-g3eae30870.
lib/themed.tcl | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/lib/themed.tcl b/lib/themed.tcl
index 351a712..85c157b 100644
--- a/lib/themed.tcl
+++ b/lib/themed.tcl
@@ -28,10 +28,7 @@ proc InitTheme {} {
}
}
- # Handle either current Tk or older versions of 8.5
- if {[catch {set theme [ttk::style theme use]}]} {
- set theme $::ttk::currentTheme
- }
+ set theme [get_current_theme]
if {[lsearch -exact {default alt classic clam} $theme] != -1} {
# Simple override of standard ttk::entry to change the field
@@ -248,7 +245,7 @@ proc tspinbox {w args} {
proc ttext {w args} {
global use_ttk
if {$use_ttk} {
- switch -- [ttk::style theme use] {
+ switch -- [get_current_theme] {
"vista" - "xpnative" {
lappend args -highlightthickness 0
-borderwidth 0
}
@@ -343,6 +340,16 @@ proc on_choosefont {familyvar sizevar font} {
set size [dict get $font -size]
}
+# Get current theme in a backwards-compatible way.
+proc get_current_theme {} {
+ # Handle either current Tk or older versions of 8.5
+ if {[catch {set theme [ttk::style theme use]}]} {
+ set theme $::ttk::currentTheme
+ }
+
+ return $theme
+}
+
# Local variables:
# mode: tcl
# indent-tabs-mode: t
^ permalink raw reply related
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 18:54 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, sandals, David.Turner
In-Reply-To: <aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org>
Am 20.12.2016 um 19:33 schrieb Johannes Sixt:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>> +void run_command_or_die(struct child_process *cmd)
>> +{
>> + if (finish_command(cmd))
>> + report_and_die(cmd, "run");
>
> And here as well.
Oh, and BTW, this one wraps only finish_command, not run_command.
-- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Johannes Sixt @ 2016-12-20 18:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqvauejvnr.fsf@gitster.mtv.corp.google.com>
Am 20.12.2016 um 19:35 schrieb Junio C Hamano:
> test_expect_success 'shortlog --committer (internal)' '
> + git checkout --orphan side &&
> + git commit --allow-empty -m one &&
> + git commit --allow-empty -m two &&
> + GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
Clever! Thank you. Will test in 12 hours.
> +
> cat >expect <<-\EOF &&
> - 3 C O Mitter
> + 2 C O Mitter
> + 1 Sin Nombre
> EOF
> git shortlog -nsc HEAD >actual &&
> test_cmp expect actual
>
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq1sx2lara.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> May I kindly ask you to make this work on Windows, too? Just
>>>
>>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>>
>> HAVENOT???
>>>
>>> on your Linux box and make it pass the tests.
>>>
>>> Thank you so much in advance.
>
> Ah, I think I am slower than my usual today.
-- >8 --
Subject: SQUASH???
Make sure the test does not depend on the result of the previous
tests; with MINGW prerequisite satisfied, a "reset to original and
rebuild" in an earlier test was skipped, resulting in different
history being tested with this and the next tests.
---
t/t4201-shortlog.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6c7c637481..9df054bf05 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -191,8 +191,14 @@ test_expect_success 'shortlog with --output=<file>' '
'
test_expect_success 'shortlog --committer (internal)' '
+ git checkout --orphan side &&
+ git commit --allow-empty -m one &&
+ git commit --allow-empty -m two &&
+ GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
+
cat >expect <<-\EOF &&
- 3 C O Mitter
+ 2 C O Mitter
+ 1 Sin Nombre
EOF
git shortlog -nsc HEAD >actual &&
test_cmp expect actual
^ permalink raw reply related
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 18:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, sandals, David.Turner
In-Reply-To: <20161219232828.5075-4-sbeller@google.com>
Am 20.12.2016 um 00:28 schrieb Stefan Beller:
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> + int i;
> + struct strbuf err = STRBUF_INIT;
> + if (cmd->git_cmd)
> + strbuf_addstr(&err, "git ");
> + for (i = 0; cmd->argv[i]; )
> + strbuf_addf(&err, "'%s'", cmd->argv[i]);
Take note that cmd is accessed here.
> + die(_("could not %s %s"), action, err.buf);
Should lego sentences not be avoided? They are not exactly translator
friendly.
Given that a lot of effort is spent elsewhere to actually *avoid* dying
in library code, this new die() is not very welcome, I must say.
Granted, you just add convenience functions here, and callers have
alternatives that do not die, but still...
> +}
> +
> int start_command(struct child_process *cmd)
> {
> int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
> return 0;
> }
>
> +void start_command_or_die(struct child_process *cmd)
> +{
> + if (start_command(cmd))
> + report_and_die(cmd, "start");
But cmd has been cleaned up at this point of call of report_and_die. The
access noted above goes to freed memory, I think.
> +}
> ...
> +void finish_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "finish");
Same here.
> +}
> ...
> +void run_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "run");
And here as well.
> +}
-- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq60melazp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> May I kindly ask you to make this work on Windows, too? Just
>>
>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>
> HAVENOT???
>>
>> on your Linux box and make it pass the tests.
>>
>> Thank you so much in advance.
Ah, I think I am slower than my usual today.
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:19 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <16b115e0-3a7e-a5c2-1526-44bbcfc97db8@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 16.12.2016 um 14:51 schrieb Jeff King:
>> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
>> index ae08b57712..6c7c637481 100755
>> --- a/t/t4201-shortlog.sh
>> +++ b/t/t4201-shortlog.sh
>> @@ -190,4 +190,17 @@ test_expect_success 'shortlog with --output=<file>' '
>> test_line_count = 3 shortlog
>> '
>>
>> +test_expect_success 'shortlog --committer (internal)' '
>> + cat >expect <<-\EOF &&
>> + 3 C O Mitter
>> + EOF
>> + git shortlog -nsc HEAD >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'shortlog --committer (external)' '
>> + git log --format=full | git shortlog -nsc >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> test_done
>>
>
> May I kindly ask you to make this work on Windows, too? Just
>
> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
HAVENOT???
>
> on your Linux box and make it pass the tests.
>
> Thank you so much in advance.
>
> -- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Johannes Sixt @ 2016-12-20 18:12 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Junio C Hamano, Git Mailing List
In-Reply-To: <20161216135141.yhas67pzfm7bxxum@sigill.intra.peff.net>
Am 16.12.2016 um 14:51 schrieb Jeff King:
> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index ae08b57712..6c7c637481 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -190,4 +190,17 @@ test_expect_success 'shortlog with --output=<file>' '
> test_line_count = 3 shortlog
> '
>
> +test_expect_success 'shortlog --committer (internal)' '
> + cat >expect <<-\EOF &&
> + 3 C O Mitter
> + EOF
> + git shortlog -nsc HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'shortlog --committer (external)' '
> + git log --format=full | git shortlog -nsc >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
May I kindly ask you to make this work on Windows, too? Just
sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
on your Linux box and make it pass the tests.
Thank you so much in advance.
-- Hannes
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Junio C Hamano @ 2016-12-20 17:41 UTC (permalink / raw)
To: Vasco Almeida; +Cc: Stefan Beller, Josh Bleecher Snyder, git@vger.kernel.org
In-Reply-To: <1482243418.2029.10.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Thanks for the report and letting me know.
> Yes, these were mistakes and lack of attention mine. It was supposed to
> call 'eval_gettext' rather than 'gettext' when \$variable interpolation
> is needed.
Thanks.
As both of the offending commits (d323c6b641 & f2d17068fd) were part
of the topic that was merged at 2703572b3a ("Merge branch
'va/i18n-even-more'", 2016-07-13), I'll queue this single patch on
top of 2703572b3a^2 (i.e. the tip of the topic).
-- >8 --
Subject: [PATCH] i18n: fix misconversion in shell scripts
An earlier series that was merged at 2703572b3a ("Merge branch
'va/i18n-even-more'", 2016-07-13) failed to use $(eval_gettext
"string with \$variable interpolation") and instead used gettext in
a few places, and ended up showing the variable names in the
message, e.g.
$ git submodule
fatal: $program_name cannot be used without a working tree.
Catch these mistakes with
$ git grep -n '[^_]gettext .*\\\$'
and fix them all to use eval_gettext instead.
Reported-by: Josh Bleecher Snyder
Acked-by: Vasco Almeida <vascomalmeida@sapo.pt>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-rebase--interactive.sh | 3 ++-
git-sh-setup.sh | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a545d92c26..c5806859f0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -467,7 +467,8 @@ update_squash_messages () {
}' <"$squash_msg".bak
} >"$squash_msg"
else
- commit_message HEAD > "$fixup_msg" || die "$(gettext "Cannot write \$fixup_msg")"
+ commit_message HEAD >"$fixup_msg" ||
+ die "$(eval_gettext "Cannot write \$fixup_msg")"
count=2
{
printf '%s\n' "$comment_char $(gettext "This is a combination of 2 commits.")"
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2eda134800..c7b2a95463 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -194,14 +194,14 @@ require_work_tree_exists () {
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
program_name=$0
- die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
+ die "$(eval_gettext "fatal: \$program_name cannot be used without a working tree.")"
fi
}
require_work_tree () {
test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true || {
program_name=$0
- die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
+ die "$(eval_gettext "fatal: \$program_name cannot be used without a working tree.")"
}
}
--
2.11.0-416-g1351c11cce
^ permalink raw reply related
* Re: [PATCH] log: support 256 colors with --graph=256colors
From: Junio C Hamano @ 2016-12-20 17:26 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161220123929.15329-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/graph.c b/graph.c
> index d4e8519..75375a1 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -78,6 +78,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)
>
> static const char **column_colors;
> static unsigned short column_colors_max;
> +static int column_colors_step;
>
> void graph_set_column_colors(const char **colors, unsigned short colors_max)
> {
> @@ -234,10 +235,24 @@ void graph_setup_line_prefix(struct diff_options *diffopt)
> }
>
>
> -struct git_graph *graph_init(struct rev_info *opt)
> +struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg)
> {
> struct git_graph *graph = xmalloc(sizeof(struct git_graph));
>
> + if (arg && !strcmp(arg, "256colors")) {
> + int i, start = 17, stop = 232;
> + column_colors_max = stop - start;
> + column_colors =
> + xmalloc((column_colors_max + 1) * sizeof(*column_colors));
> + for (i = start; i < stop; i++) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(&sb, "\033[38;5;%dm", i);
> + column_colors[i - start] = strbuf_detach(&sb, NULL);
> + }
> + column_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);
> + /* ignore the closet 16 colors on either side for the next line */
> + column_colors_step = 16;
> + }
So you pre-fill a table of colors with 232-17=215 slots. Is the
idea that it is a co-prime with column_colors_step which is set to
16 so that going over the table with wraparound will cover all its
elements?
> @@ -382,6 +397,20 @@ static unsigned short graph_get_current_column_color(const struct git_graph *gra
> */
> static void graph_increment_column_color(struct git_graph *graph)
> {
> + if (column_colors_step) {
> + static int random_initialized;
> + int v;
> +
> + if (!random_initialized) {
> + srand((unsigned int)getpid());
> + random_initialized = 1;
> + }
> + v = rand() % (column_colors_max - column_colors_step * 2);
> + graph->default_column_color += column_colors_step + v;
> + graph->default_column_color %= column_colors_max;
> + return;
> + }
> +
> graph->default_column_color = (graph->default_column_color + 1) %
> column_colors_max;
> }
This is too ugly to live as-is for two reasons.
- Do you really need rand()? Doesn't this frustrate somebody who
runs the same "git log" in two terminals in order to view an
overly tall graph, expecting both commands that were started with
the same set of arguments to paint the same line in the same
color?
- Even if you needed rand(), you should be able to factor out
computation of V so that the function does not need an early
return that hints totally different processing for two codepaths.
static void graph_increment_column_color(struct git_graph *g)
{
int next;
if (! 256-color in use) {
next = 1;
} else {
do whatever to compute your v;
next = v + column_colors_step;
}
g->default_column_color =
(g->default_column_color + v) / column_colors_max;
}
Or something like that, perhaps?
^ permalink raw reply
* Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes
From: Junio C Hamano @ 2016-12-20 17:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Max Kirillov, Karsten Blees, git
In-Reply-To: <alpine.DEB.2.20.1612201610270.54750@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Max,
>
> On Mon, 19 Dec 2016, Max Kirillov wrote:
>
>> UNICODE_STRING::Length field means size of buffer in bytes[1], despite
>> of buffer itself being array of wchar_t. Because of that terminating
>> zero is placed twice as far. Fix it.
>
> This commit message needs to be wrapped at <= 76 columns per row.
> ...
> Very good, thank you for fixing my mistake! I verified locally that it
> does exactly the right thing with your patch.
Thanks, both. I'll queue this like so.
-- >8 --
From: Max Kirillov <max@max630.net>
Date: Mon, 19 Dec 2016 23:32:00 +0200
Subject: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes
UNICODE_STRING::Length field means size of buffer in bytes[1],
despite of buffer itself being array of wchar_t. Because of that
terminating zero is placed twice as far. Fix it.
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx
Signed-off-by: Max Kirillov <max@max630.net>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
compat/winansi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 3be60ce1c6..6b4f736fdc 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -553,7 +553,7 @@ static void detect_msys_tty(int fd)
buffer, sizeof(buffer) - 2, &result)))
return;
name = nameinfo->Name.Buffer;
- name[nameinfo->Name.Length] = 0;
+ name[nameinfo->Name.Length / sizeof(*name)] = 0;
/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
base-commit: f7f90e0f4f58d493242078d17c0eba41dd3f1f79
--
2.11.0-416-g1351c11cce
^ permalink raw reply related
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Junio C Hamano @ 2016-12-20 16:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, Pranit Bauva
In-Reply-To: <alpine.DEB.2.20.1612201732160.54750@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> That code works because winansi_get_osfhandle() is in winansi.c, where its
> call to isatty() is *not* redirected to winansi_isatty(). Good.
> ...
> My plan was actually ...
> ...
> Let's just clean up all of this in one go.
I take this to mean that I should hold off applying/merging j6t's
one liner and wait for your counterproposal tested by j6t. If I
misread you please let me know.
Thanks for working well together, as always.
^ permalink raw reply
* Re: [PATCH] log: support 256 colors with --graph=256colors
From: Jeff King @ 2016-12-20 16:57 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161220123929.15329-1-pclouds@gmail.com>
On Tue, Dec 20, 2016 at 07:39:29PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> I got mad after tracing two consecutive red history lines in `git log
> --graph --oneline` back to their merge points, far far away. Yeah
> probably should fire up tig, or gitk or something.
>
> This may sound like a good thing to add, but I don't know how good it
> is compared to the good old 16 color palette, yet as I haven't tried it
> for long since it's just written.
Hmm. At some point the colors become too close together to be easily
distinguishable. In your code you have:
> + if (arg && !strcmp(arg, "256colors")) {
> + int i, start = 17, stop = 232;
> + column_colors_max = stop - start;
> + column_colors =
> + xmalloc((column_colors_max + 1) * sizeof(*column_colors));
> + for (i = start; i < stop; i++) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(&sb, "\033[38;5;%dm", i);
> + column_colors[i - start] = strbuf_detach(&sb, NULL);
> + }
> + column_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);
> + /* ignore the closet 16 colors on either side for the next line */
> + column_colors_step = 16;
> + }
So you step by 16, over a set of 215 colors. That seems to give only 13
colors, versus the original 16. :)
I know that is a simplification. If you wrap around, then you get your
13 colors, and then another 13 colors that aren't _quite_ the same, and
so on, until you've used all 256. I'm just not sure if the 1st and 14th
color would be visually different enough for it to matter (I admit I
didn't do any experiments, though).
> ---graph::
> +--graph[=<options>]::
> Draw a text-based graphical representation of the commit history
> on the left hand side of the output. This may cause extra lines
> to be printed in between commits, in order for the graph history
I wonder if we would ever want another use for "--graph=foo". I guess
any such thing could fall under the name of "graph options", and we'd
end up with "--graph=256colors,unicode" or something like that.
I do suspect people would want a config option for this, though. I.e.,
you'd want to enable it all the time if you have a terminal which can
handle 256 colors, not just for a particular invocation.
-Peff
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Schindelin @ 2016-12-20 16:53 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Pranit Bauva
In-Reply-To: <d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org>
Hi Hannes,
On Sun, 18 Dec 2016, Johannes Sixt wrote:
> What do you think?
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index ba360be69b..1748d17777 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
>
> int winansi_isatty(int fd)
> {
> - int res = isatty(fd);
> -
> - if (res) {
> + switch (fd) {
> + case 0:
> /*
> * Make sure that /dev/null is not fooling Git into believing
> * that we are connected to a terminal, as "_isatty() returns a
> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)
> *
> * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
> */
> - HANDLE handle = winansi_get_osfhandle(fd);
> - if (fd == STDIN_FILENO) {
> + {
> + HANDLE handle = (HANDLE)_get_osfhandle(fd);
> DWORD dummy;
>
> - if (!GetConsoleMode(handle, &dummy))
> - res = 0;
> - } else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
> - CONSOLE_SCREEN_BUFFER_INFO dummy;
> -
> - if (!GetConsoleScreenBufferInfo(handle, &dummy))
> - res = 0;
> + return !!GetConsoleMode(handle, &dummy);
> }
> + case 1:
> + return !!hconsole1;
> + case 2:
> + return !!hconsole2;
> }
>
> - return res;
> + return isatty(fd);
> }
>
> void winansi_init(void)
I think that would break running Git in Git Bash (i.e. MinTTY) ;-)
Let me try to come up with a patch series starting from your patch. We
need
- to abandon the _pioinfo hack
- to make isatty() work correctly with /dev/null
- to make isatty() work correctly in CMD
- to make isatty() work correctly in MinTTY (i.e. with MSYS2 pipes instead
of Consoles)
I think we can have it all.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Schindelin @ 2016-12-20 16:50 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Pranit Bauva
In-Reply-To: <ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org>
Hi Hannes,
On Sun, 18 Dec 2016, Johannes Sixt wrote:
> The code in winansi.c emulates ANSI escape sequences when Git is
> connected to the "real" windows console, CMD.exe. The details are
> outline in eac14f8909d9 (Win32: Thread-safe windows console output,
> 2012-01-14). Essentially, it plugs a pipe between C code and the actual
> console output handle.
>
> This commit also added an override for isatty(), but it was made
> unnecessary by fcd428f4a952 (Win32: fix broken pipe detection,
> 2012-03-01).
>
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.
Thank you.
> diff --git a/compat/winansi.c b/compat/winansi.c
> index cb725fb02f..ba360be69b 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -586,7 +586,7 @@ int winansi_isatty(int fd)
> *
> * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
> */
> - HANDLE handle = (HANDLE)_get_osfhandle(fd);
> + HANDLE handle = winansi_get_osfhandle(fd);
That code works because winansi_get_osfhandle() is in winansi.c, where its
call to isatty() is *not* redirected to winansi_isatty(). Good.
My plan was actually to clean up the "magic" detect_msys_tty() code: it
messes with internals of the MSVC runtime that are no longer the same in
the Universal Runtime (UCRT), and hence we already had to come up with a
different way to detect an MSYS2 pipe. My preference would be to merge
that logic into winansi_isatty() and abandon the _pioinfo hack.
Let's just clean up all of this in one go.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCHv2 0/7] Fix and generalize version sort reordering
From: Jeff King @ 2016-12-20 16:49 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Leho Kraav,
git
In-Reply-To: <CAM0VKjmDDKgYCvtbwpx=GcwRENzvSDLW_Xhia3btdeMjtAjAvg@mail.gmail.com>
On Tue, Dec 20, 2016 at 09:50:42AM +0100, SZEDER Gábor wrote:
> > It just seems like the whole thing would conceptually easier if we
> > pre-parsed the versions into a sequence of elements, then the comparison
> > between any two elements would just walk that sequence. The benefit
> > there is that you can implement whatever rules you like for the parsing
> > (like "prefer longer suffixes to shorter"), but you know the comparison
> > will always be consistent.
>
> I considered parsing tagnames into prefix, version number and suffix,
> and then work from that, but decided against it.
>
> versioncmp() is taken from glibc, so I assume that it's thoroughly
> tested, even in corner cases (e.g. multiple leading zeros).
> Furthermore, I think it's a good thing that by default (i.e. without
> suffix reordering) our version sort orders the same way as glibc's
> version sort does. Introducing a different algorithm would risk bugs
> in the more subtle cases.
Fair enough. If it's working, I agree there is risk in changing things.
And if you're willing to deal with the bugs, then I'm happy to stand
back. :)
> Then there are all the weird release suffixes out there, and I didn't
> want to decide on a policy for splitting them sanely; don't know
> whether there exist any universal rules for this splitting at
> all. E.g. one of the packages here has the following version (let's
> ignore the fact that because of the '~' this is an invalid refname in
> git):
I have a hunch that any policy you'd have to set for splitting is going
to end up becoming a policy you'll have to use when comparing (or you
risk violating the transitivity of your comparison function).
But that's just a hunch, not a proof. Again, I'm happy to defer to you
if you're the one working on it.
-Peff
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Jeff King @ 2016-12-20 16:45 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Kyle J. McKay, Jonathan Tan, Junio C Hamano, Git mailing list
In-Reply-To: <alpine.DEB.2.20.1612201511480.54750@virtualbox>
On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
> > On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
> >
> > >ACK. I noticed this problem (and fixed it independently as a part of a
> > >huge patch series I did not get around to submit yet) while trying to
> > >get Git to build correctly with Visual C.
> >
> > Does this mean that Dscho and I are the only ones who add -DNDEBUG for
> > release builds? Or are we just the only ones who actually run the test
> > suite on such builds?
>
> It seems you and I are for the moment the only ones bothering with running
> the test suite on release builds.
I wasn't aware anybody actually built with NDEBUG at all. You'd have to
explicitly ask for it via CFLAGS, so I assume most people don't.
Certainly I never have when deploying to GitHub's cluster (let alone my
personal use), and I note that the Debian package also does not.
So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?". One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to assert()
in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).
I do notice that we set NDEBUG for nedmalloc, though if I am reading the
Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes
From: Johannes Schindelin @ 2016-12-20 15:16 UTC (permalink / raw)
To: Max Kirillov; +Cc: Junio C Hamano, Karsten Blees, git
In-Reply-To: <1482183120-21592-1-git-send-email-max@max630.net>
Hi Max,
On Mon, 19 Dec 2016, Max Kirillov wrote:
> UNICODE_STRING::Length field means size of buffer in bytes[1], despite
> of buffer itself being array of wchar_t. Because of that terminating
> zero is placed twice as far. Fix it.
This commit message needs to be wrapped at <= 76 columns per row.
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> Access outside of buffer was very unlikely (for that user needed to redirect
> standard fd to a file with path longer than ~250 symbols), it still did not
> seem to do any harm, and otherwise it did not break because only substring is
> checked, but it was still incorrect.
Very good, thank you for fixing my mistake! I verified locally that it
does exactly the right thing with your patch.
ACK,
Dscho
^ 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