* Re: [PATCH 0/5] git check-ref-format --stdin --report-errors
From: Michael Haggerty @ 2016-12-20 7:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ian Jackson, git
In-Reply-To: <xmqqlgvbpyku.fsf@gitster.mtv.corp.google.com>
On 12/19/2016 07:23 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Especially given that the output is not especially machine-readable, it
>> might be more consistent with other commands to call the new feature
>> `--verbose` rather than `--report-errors`.
>
> Don't we instead want to structure the output to be machine-readable
> instead, given that check-ref-format is a very low level plumbing
> command that is primarily meant for scriptors?
Of course that would be the ideal. Let's think about what it would look
like. Given that the very purpose of the program is to decide whether
its inputs are reasonable reference names or not, it would be important
to make it bulletproof:
* It could be fed some ugly garbage
* It could be used for security-relevant checks
One obvious choice would be to use NUL separators, but that would make
the output mostly unreadable to humans.
Another would be to use LF to terminate each line of output, like
ok TAB refs/heads/foo LF
bad TAB refs/heads/bad SP name@@.lock LF
For the LF-terminated `--stdin` input, this should be unambiguous.
However, it wouldn't necessarily work for arguments passed in via the
command line, for for slight variations on `--stdin` like if we were to
add a `-z` option to allow the input to be NUL-terminated.
The 100% solution would probably be to support language-specific
quoting, like the `--shell`/`--perl`/`--python`/`--tcl` options accepted
by `for-each-ref`, probably with a fifth option for NUL-terminated
output. And it should probably also support a `-z` option to make its
input NUL-separated. Pretty much all of the infrastructure is already
there in `quote.h` and `quote.c`, and the option-parsing could be
cribbed from `builtin/for-each-ref.c`, so it wouldn't even be *that*
much work to implement.
Michael
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Michael Haggerty @ 2016-12-20 8:03 UTC (permalink / raw)
To: Philip Oakley, Paul Mackerras; +Cc: Git List
In-Reply-To: <CB83F601FBED482582F20099849D661A@PhilipOakley>
On 12/19/2016 11:53 PM, Philip Oakley wrote:
> From: "Michael Haggerty" <mhagger@alum.mit.edu>
>> This patch series changes a bunch of details about how remote-tracking
>> references are rendered in the commit list of gitk:
>>
> [...]
>> * Introduce a separate constant to specify the background color used
>> for the branch name part of remote-tracking references, to allow it
>> to differ from the color used for local branches (which by default
>> is bright green).
>>
>> * Change the default background colors for remote-tracking branches to
>> light brown and brown (formerly they were pale orange and bright
>> green).
>>
>> I understand that the colors of pixels on computer screens is an even
>> more emotional topic that that of bikesheds, so I implemented the last
>> change as a separate commit, the last one in the series. Feel free to
>> drop it if you don't want the default color change.
>
> Just to say that there was an issue with the bright green (lime) a while
> back when 'green' changed its colour.
>
> dscho reported in
> (https://github.com/git-for-windows/git/issues/300#issuecomment-133702654
> 26 Aug 2015)
>
> "[T]his is a change in Tk 8.6 described here (http://wiki.tcl.tk/1424):
> From
> Tcl/Tk 8.6 on, Tk uses Web colours instead of X11 ones, where they
> conflict."
>
> In particular the old bright green version of 'green' became a darker
> green,
> with the old colour becoming named lime.
>
> For me, I needed to change my colour scheme (to a lime) as I could not read
> the refs against the darker colour.
>
> Anyway, that's the background as I know it.
Thanks for the context. In fact, it was this color change that got me
looking at this code in the first place.
For anyone still suffering from the "green background of branch labels
in gitk got too dark" problem:
The default color in gitk was made lighter again in
66db14c "gitk: Color name update", 2015-10-25
But your own configuration might still have "green" written in it, from
the days when "green" was still bright. If so, change the value for
`headbgcolor` to `lime` in `~/.config/git/gitk`.
It would be nice if these colors were adjustable via the "Preferences ->
Colors" dialog.
Michael
^ permalink raw reply
* [ANNOUNCE] git-cinnabar 0.4.0 release candidate 2
From: Mike Hommey @ 2016-12-20 8:47 UTC (permalink / raw)
To: git
Hi,
Git-cinnabar is a git remote helper to interact with mercurial
repositories. It allows to clone, pull and push from/to mercurial remote
repositories, using git.
Code on https://github.com/glandium/git-cinnabar
This release on
https://github.com/glandium/git-cinnabar/releases/tag/0.4.0rc2
[ Previous announcements:
http://marc.info/?l=git&m=148038131415810
http://marc.info/?l=git&m=146943537220142
http://marc.info/?l=git&m=146762932928309 (...)]
What's new since 0.4.0rc?
- /!\ Warning /!\ If you have been using a version of the `release`
branch between 0.4.0rc and 0.4.0rc2 (more precisely, in the range
0335aa1432bdb0a8b5bdbefa98f7c2cd95fc72d2^..0.4.0rc2^), *and* used `git
cinnabar download` *and* run on Mac or Windows, please ensure your
mercurial clones have not been corrupted by case-sensitivity issues by
running `git cinnabar fsck --manifests`. If they contain sha1
mismatches, please reclone.
- Updated git to 2.11.0 for cinnabar-helper
- Improvements to the `git cinnabar download` command
- Various small code cleanups
- Improvement to the experimental support for pushing merges.
Mike
^ permalink raw reply
* Re: [PATCHv2 0/7] Fix and generalize version sort reordering
From: SZEDER Gábor @ 2016-12-20 8:50 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Leho Kraav,
git
In-Reply-To: <20161214170852.bzh5pyl4bov6rwbt@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 6:08 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:
>
>> > With my patches it looks like this:
>> >
>> > $ git -c versionsort.prereleasesuffix=-pre \
>> > -c versionsort.prereleasesuffix=-prerelease \
>> > tag -l --sort=version:refname
>> > v1.0.0-prerelease1
>> > v1.0.0-pre1
>> > v1.0.0-pre2
>> > v1.0.0
>>
>> And when there happen to be more than one matching suffixes starting
>> at the same earliest position, then we should pick the longest of
>> them. The new patch 6/7 implements this behavior.
>
> The whole approach taken by the suffix code (before your patches) leaves
> me with the nagging feeling that the comparison is not always going to
> be transitive (i.e., that "a < b && b < c" does not always imply "a <
> c"), which is going to cause nonsensical sorting results.
>
> And that may be part of the issue your 6/7 fixes.
>
> I spent some time playing with this the other day, though, and couldn't
> come up with a specific example that fails the condition above.
>
> 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.
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):
1.1.0~rc1-2ubuntu7-1linuxmint1
Now, it's clear that the version number is "1.1.0", and the user
should configure the suffix "~rc" for prerelease reordering. But what
about the rest? How should we split it "into a sequence of elements",
is it { "1.1.0", "~rc1", "-2ubuntu7", "-1linuxmint1" } or { "1.1.0",
"~rc1-2", "ubuntu7-1", "linuxmint1" }?
What if there is a hard-working developer who is involved in a lot of
Debian derivatives (and derivatives of derivatives...), and, for
whatever reason, wants to put derivative-specific versions in a
particular order? With my series, or conceptually even with master if
it weren't buggy, it's possible to specify the order of suffixes of
suffixes, and that dev could do this:
$ git -c versionsort.suffix=-rc
-c versionsort.suffix=linuxmint
-c versionsort.suffix=YADoaDD
tag -l --sort=version:refname
'1.1.0*'
1.1.0-rc1-2ubuntu7-1linuxmint1
1.1.0-rc1-2ubuntu7-1YADoaDD2
1.1.0
1.1.0-2ubuntu7-1linuxmint1
1.1.0-2ubuntu7-1YADoaDD2
and would get Linux Mint-specific tags before "Yet Another Derivative
of a Debian Derivative"-specific ones. Not sure whether this is
relevant in practice, but I think it's a nice property nonetheless.
(Btw, just for fun, I also found a package version
2.0.0~beta2+isreally1.8.6-0ubuntu1. "isreally". Oh yeah :)
> It would also be more efficient, I think (it seems like the sort is
> O(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the
> comparator). Though that probably doesn't matter much in practice.
I don't think there will be more than only a few configured suffixes
in any repository. However, if you consider O as "number of
starts_with() invocations", then there is an additional suffix_length
factor. But then again, these suffixes tend to be short.
> I dunno. I think maybe your 6/7 has converged on an equivalent behavior.
> And I am certainly not volunteering to re-write it, so if what you have
> works, I'm not opposed to it.
>
> -Peff
^ permalink raw reply
* Re: [ANNOUNCE] git-cinnabar 0.4.0 release candidate 2
From: Mike Hommey @ 2016-12-20 8:56 UTC (permalink / raw)
To: git
In-Reply-To: <20161220084744.xdy2vwvoyudsubqn@glandium.org>
On Tue, Dec 20, 2016 at 05:47:44PM +0900, Mike Hommey wrote:
> Hi,
>
> Git-cinnabar is a git remote helper to interact with mercurial
> repositories. It allows to clone, pull and push from/to mercurial remote
> repositories, using git.
>
> Code on https://github.com/glandium/git-cinnabar
> This release on
> https://github.com/glandium/git-cinnabar/releases/tag/0.4.0rc2
>
> [ Previous announcements:
> http://marc.info/?l=git&m=148038131415810
> http://marc.info/?l=git&m=146943537220142
> http://marc.info/?l=git&m=146762932928309 (...)]
>
> What's new since 0.4.0rc?
>
> - /!\ Warning /!\ If you have been using a version of the `release`
> branch between 0.4.0rc and 0.4.0rc2 (more precisely, in the range
> 0335aa1432bdb0a8b5bdbefa98f7c2cd95fc72d2^..0.4.0rc2^), *and* used `git
> cinnabar download` *and* run on Mac or Windows, please ensure your
> mercurial clones have not been corrupted by case-sensitivity issues by
> running `git cinnabar fsck --manifests`. If they contain sha1
> mismatches, please reclone.
Oh, and please first run `git cinnabar download` with this new version.
Mike
^ permalink raw reply
* Re: [PATCH] config.c: handle error case for fstat() calls
From: Duy Nguyen @ 2016-12-20 9:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, josharian
In-Reply-To: <xmqqtw9zpz0c.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 19, 2016 at 10:14:27AM -0800, Junio C Hamano wrote:
> Shouldn't the error-return path in the second hunk rollback the
> lockfile to clean after itself? The existing "Oh, we cannot chmod
> to match the original" check that comes immediately after shares the
> same issue, so this is not a new problem, but making an existing one
> worse.
OK. How about two more patches on top (or bottom, does not matter)?
The second one should fix this. The first is sort of "good to do".
[PATCH 1/2] config.c: rename label unlock_and_out
[PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
--
Duy
^ permalink raw reply
* [PATCH 1/2] config.c: rename label unlock_and_out
From: Nguyễn Thái Ngọc Duy @ 2016-12-20 9:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, josharian, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161220094746.GA3917@ash>
There are two ways to unlock a file: commit, or revert. Rename it to
commit_and_out to avoid confusion.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 4973256..505e0d0 100644
--- a/config.c
+++ b/config.c
@@ -2416,7 +2416,7 @@ int git_config_rename_section_in_file(const char *config_filename,
if (!(config_file = fopen(config_filename, "rb"))) {
/* no config file means nothing to rename, no error */
- goto unlock_and_out;
+ goto commit_and_out;
}
if (fstat(fileno(config_file), &st) == -1) {
@@ -2478,7 +2478,7 @@ int git_config_rename_section_in_file(const char *config_filename,
}
}
fclose(config_file);
-unlock_and_out:
+commit_and_out:
if (commit_lock_file(lock) < 0)
ret = error_errno("could not write config file %s",
config_filename);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
From: Nguyễn Thái Ngọc Duy @ 2016-12-20 9:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, josharian, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161220094836.4131-1-pclouds@gmail.com>
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.
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;
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH v1] git-p4: fix git-p4.pathEncoding for removed files
From: Luke Diamand @ 2016-12-20 11:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Git Users
In-Reply-To: <xmqq37hjobf6.fsf@gitster.mtv.corp.google.com>
On 19 December 2016 at 21:29, Junio C Hamano <gitster@pobox.com> wrote:
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> In a9e38359e3 we taught git-p4 a way to re-encode path names from what
>> was used in Perforce to UTF-8. This path re-encoding worked properly for
>> "added" paths. "Removed" paths were not re-encoded and therefore
>> different from the "added" paths. Consequently, these files were not
>> removed in a git-p4 cloned Git repository because the path names did not
>> match.
>>
>> Fix this by moving the re-encoding to a place that affects "added" and
>> "removed" paths. Add a test to demonstrate the issue.
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>
> Thanks.
>
> The above description makes me wonder what happens to "modified"
> paths, but presumably they are handled in a separate codepath? Or
> does this also cover not just "removed" but also paths with any
> change?
>
> Luke, does this look good?
I'm not totally sure. In the previous version the conversion happened
in streamOneP4File(). There is a counterpart to this,
streamOneP4Deletion() which would seem like the callpoint that needs
to know about this.
The change puts the logic into stripRepoPath() instead, which is
indeed called from both of those functions (good), but also from
splitFilesIntoBranches(), but only if self.useClientSpec is set. That
function only gets used if we're doing the automatic branch detection
logic, so it's possible that this code might now be broken and we
wouldn't know.
Lars, what do you think? Other than the above, the change looks good,
so it may all be fine.
(As an aside, this is the heart of the code that's going to need some
careful rework if/when we ever move to Python3).
Luke
^ permalink raw reply
* Re: [PATCH v1] git-p4: add diff/merge properties to .gitattributes for GitLFS files
From: Luke Diamand @ 2016-12-20 11:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Git Users
In-Reply-To: <xmqqy3zbmwu5.fsf@gitster.mtv.corp.google.com>
On 19 December 2016 at 21:29, Junio C Hamano <gitster@pobox.com> wrote:
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> The `git lfs track` command generates a .gitattributes file with diff
>> and merge properties [1]. Set the same .gitattributes format for files
>> tracked with GitLFS in git-p4.
>>
>> [1] https://github.com/git-lfs/git-lfs/blob/v1.5.3/commands/command_track.go#L121
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>
> Thanks. Luke, does this look ok?
Yes, looks good to me.
Luke
>
>>
>> Notes:
>> Base Commit: d1271bddd4 (v2.11.0)
>> Diff on Web: https://github.com/git/git/compare/d1271bddd4...larsxschneider:e045b3d5c8
>> Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-lfs-attributes-v1 && git checkout e045b3d5c8
>>
>> git-p4.py | 4 ++--
>> t/t9824-git-p4-git-lfs.sh | 24 ++++++++++++------------
>> 2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index fd5ca52462..87b6932c81 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1098,10 +1098,10 @@ class GitLFS(LargeFileSystem):
>> '# Git LFS (see https://git-lfs.github.com/)\n',
>> '#\n',
>> ] +
>> - ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
>> + ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
>> for f in sorted(gitConfigList('git-p4.largeFileExtensions'))
>> ] +
>> - ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
>> + ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
>> for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)
>> ]
>> )
>> diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
>> index 110a7e7924..1379db6357 100755
>> --- a/t/t9824-git-p4-git-lfs.sh
>> +++ b/t/t9824-git-p4-git-lfs.sh
>> @@ -81,9 +81,9 @@ test_expect_success 'Store files in LFS based on size (>24 bytes)' '
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - /file2.dat filter=lfs -text
>> - /file4.bin filter=lfs -text
>> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
>> + /file2.dat filter=lfs diff=lfs merge=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -109,7 +109,7 @@ test_expect_success 'Store files in LFS based on size (>25 bytes)' '
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - /file4.bin filter=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -135,7 +135,7 @@ test_expect_success 'Store files in LFS based on extension (dat)' '
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - *.dat filter=lfs -text
>> + *.dat filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -163,8 +163,8 @@ test_expect_success 'Store files in LFS based on size (>25 bytes) and extension
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - *.dat filter=lfs -text
>> - /file4.bin filter=lfs -text
>> + *.dat filter=lfs diff=lfs merge=lfs -text
>> + /file4.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -199,8 +199,8 @@ test_expect_success 'Remove file from repo and store files in LFS based on size
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - /file2.dat filter=lfs -text
>> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
>> + /file2.dat filter=lfs diff=lfs merge=lfs -text
>> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -237,8 +237,8 @@ test_expect_success 'Add .gitattributes and store files in LFS based on size (>2
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - /file2.dat filter=lfs -text
>> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
>> + /file2.dat filter=lfs diff=lfs merge=lfs -text
>> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
>> @@ -278,7 +278,7 @@ test_expect_success 'Add big files to repo and store files in LFS based on compr
>> #
>> # Git LFS (see https://git-lfs.github.com/)
>> #
>> - /file6.bin filter=lfs -text
>> + /file6.bin filter=lfs diff=lfs merge=lfs -text
>> EOF
>> test_path_is_file .gitattributes &&
>> test_cmp expect .gitattributes
^ permalink raw reply
* [PATCH] log: support 256 colors with --graph=256colors
From: Nguyễn Thái Ngọc Duy @ 2016-12-20 12:39 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
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.
BTW anyone interested in bringing this type [1] of --graph to git? I
tried the unicode box characters, but the vertical lines do not join
with diagonal ones, making the graph a bit ugly (even though it's
still better than ascii version)
[1] https://github.com/magit/magit/issues/495#issuecomment-17480757
Documentation/rev-list-options.txt | 5 ++++-
graph.c | 31 ++++++++++++++++++++++++++++++-
graph.h | 8 +++++++-
revision.c | 4 ++++
4 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5da7cf5..0a0c2f3 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -825,7 +825,7 @@ you would get an output like this:
-xxxxxxx... 1st on a
-----------------------------------------------------------------------
---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
@@ -836,6 +836,9 @@ This enables parent rewriting, see 'History Simplification' below.
+
This implies the `--topo-order` option by default, but the
`--date-order` option may also be specified.
++
+The only supported option is `256colors` which allows more than 16
+colors for drawing the commit history.
--show-linear-break[=<barrier>]::
When --graph is not used, all history branches are flattened
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;
+ }
if (!column_colors)
graph_set_column_colors(column_colors_ansi,
column_colors_ansi_max);
@@ -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;
}
diff --git a/graph.h b/graph.h
index af62339..8069aa4 100644
--- a/graph.h
+++ b/graph.h
@@ -40,7 +40,13 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);
/*
* Create a new struct git_graph.
*/
-struct git_graph *graph_init(struct rev_info *opt);
+struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg);
+
+static inline struct git_graph *graph_init(struct rev_info *opt)
+{
+ return graph_init_with_options(opt, NULL);
+}
+
/*
* Update a git_graph with a new commit.
diff --git a/revision.c b/revision.c
index b37dbec..07bea54 100644
--- a/revision.c
+++ b/revision.c
@@ -1933,6 +1933,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->topo_order = 1;
revs->rewrite_parents = 1;
revs->graph = graph_init(revs);
+ } else if (skip_prefix(arg, "--graph=", &arg)) {
+ revs->topo_order = 1;
+ revs->rewrite_parents = 1;
+ revs->graph = graph_init_with_options(revs, arg);
} else if (!strcmp(arg, "--root")) {
revs->show_root_diff = 1;
} else if (!strcmp(arg, "--no-commit-id")) {
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCHv2] Tweak help auto-correct phrasing.
From: Marc Branchaud @ 2016-12-20 14:02 UTC (permalink / raw)
To: git; +Cc: Kaartic Sivaraam, Chris Packham, Alex Riesen, Junio C Hamano
In-Reply-To: <xmqqpoknmv7d.fsf@gitster.mtv.corp.google.com>
When auto-correct is enabled, an invalid git command prints a warning and
a continuation message, which differs depending on whether or not
help.autoCorrect is positive or negative.
With help.autoCorrect = 15:
WARNING: You called a Git command named 'lgo', which does not exist.
Continuing under the assumption that you meant 'log'
in 1.5 seconds automatically...
With help.autoCorrect < 0:
WARNING: You called a Git command named 'lgo', which does not exist.
Continuing under the assumption that you meant 'log'
The continuation message's phrasing is awkward. This commit cleans it up.
As a bonus, we now use full-sentence strings which make translation easier.
With help.autoCorrect = 15:
WARNING: You called a Git command named 'lgo', which does not exist.
Continuing in 1.5 seconds, assuming that you meant 'log'.
With help.autoCorrect < 0:
WARNING: You called a Git command named 'lgo', which does not exist.
Continuing under the assumption that you meant 'log'.
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
Writing the commit message was more work than the commit! :)
M.
help.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/help.c b/help.c
index 53e2a67e00..fc56aa2d76 100644
--- a/help.c
+++ b/help.c
@@ -381,12 +381,18 @@ const char *help_unknown_cmd(const char *cmd)
clean_cmdnames(&main_cmds);
fprintf_ln(stderr,
_("WARNING: You called a Git command named '%s', "
- "which does not exist.\n"
- "Continuing under the assumption that you meant '%s'"),
- cmd, assumed);
- if (autocorrect > 0) {
- fprintf_ln(stderr, _("in %0.1f seconds automatically..."),
- (float)autocorrect/10.0);
+ "which does not exist."),
+ cmd);
+ if (autocorrect < 0)
+ fprintf_ln(stderr,
+ _("Continuing under the assumption that "
+ "you meant '%s'."),
+ assumed);
+ else {
+ fprintf_ln(stderr,
+ _("Continuing in %0.1f seconds, "
+ "assuming that you meant '%s'."),
+ (float)autocorrect/10.0, assumed);
sleep_millisec(autocorrect * 100);
}
return assumed;
--
2.11.0.1.g75fa99b
^ permalink raw reply related
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Johannes Schindelin @ 2016-12-20 14:12 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Jeff King, Jonathan Tan, Junio C Hamano, Git mailing list
In-Reply-To: <A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com>
Hi Kyle,
On Mon, 19 Dec 2016, Kyle J. McKay 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.
Ciao,
Johannes
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Vasco Almeida @ 2016-12-20 14:16 UTC (permalink / raw)
To: Junio C Hamano, Stefan Beller; +Cc: Josh Bleecher Snyder, git@vger.kernel.org
In-Reply-To: <xmqqfuljod70.fsf@gitster.mtv.corp.google.com>
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. Junio Hamano has the right answer for these errors.
A Seg, 19-12-2016 às 12:50 -0800, Junio C Hamano escreveu:
> Subject: rebase -i: fix mistaken i18n
>
> f2d17068fd ("i18n: rebase-interactive: mark comments of squash for
> translation", 2016-06-17) attempted to apply sh-i18n and failed to
> use $(eval_gettext "string with \$variable interpolation").
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> git-rebase--interactive.sh | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 41fd374c72..96865b2375 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -437,7 +437,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.")"
I agree with this fix. Perhaps indent the second line to be easier on
the eyes?:
> + commit_message HEAD >"$fixup_msg" ||
> + die "$(eval_gettext "Cannot write \$fixup_msg")"
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2016-12-20 15:01 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <cover.1482164633.git.mhagger@alum.mit.edu>
On 2016-12-19 11:44 AM, Michael Haggerty wrote:
> This patch series changes a bunch of details about how remote-tracking
> references are rendered in the commit list of gitk:
Thanks for this! I like the new, compact look very much!
That said, I remember when I was a new git user and I leaned heavily on
gitk to understand how references worked. It was particularly
illuminating to see the remote references distinctly labeled, and the
fact that they were "remotes/origin/foo" gave me an Aha! moment where I
came to understand that the refs hierarchy is more flexible than just
the conventions coded into git itself. I eventually felt free to create
my own, private ref hierarchies.
I am in no way opposed to this series. I just wanted to point out that
there was some utility in those labels. It makes me think that it might
be worthwhile for gitk to have a "raw-refs" mode, that shows the full
"refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
could be a useful teaching tool for git.
> * Omit the "remote/" prefix on normal remote-tracking references. They
If you re-roll, s:remote/:remotes/:.
> are already distinguished via their two-tone rendering and (usually)
> longer names, and this change saves a lot of visual clutter and
> horizontal space.
>
> * Render remote-tracking references that have more than the usual
> three slashes like
>
> origin/foo/bar
> ^^^^^^^
>
> rather than
>
> origin/foo/bar (formerly remotes/origin/foo/bar)
> ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
>
> , where the indicated part is the prefix that is rendered in a
> different color. Usually, such a reference represents a remote
> branch that contains a slash in its name, so the new split more
> accurately portrays the separation between remote name and remote
> branch name.
*Love* this change! :)
> * Introduce a separate constant to specify the background color used
> for the branch name part of remote-tracking references, to allow it
> to differ from the color used for local branches (which by default
> is bright green).
>
> * Change the default background colors for remote-tracking branches to
> light brown and brown (formerly they were pale orange and bright
> green).
Please don't change the remotebgcolor default.
Also, perhaps the default remoterefbgcolor should be
set remoterefbgcolor $headbgcolor
?
I say this because when I applied the series, without the last patch, I
was miffed that the remote/ref colour had changed.
Plus I think there's utility in having local and remote branch names in
the same colour, again especially for new users. It helps emphasize
their similarities, and demystify some of git's internal magic.
M.
> I understand that the colors of pixels on computer screens is an even
> more emotional topic that that of bikesheds, so I implemented the last
> change as a separate commit, the last one in the series. Feel free to
> drop it if you don't want the default color change.
>
> Along the way, I did a bunch of refactoring in the area to make these
> changes easier, and introduced a constant for the background color of
> "other" references so that it can also be adjusted by users.
>
> (Unfortunately, these colors can only be adjusted by editing the
> configuration file. Someday it would be nice to allow them to be
> configured via the preferences dialog.)
>
> It's been a while since I've written any Tcl code, so I apologize in
> advance for any howlers :-)
>
> This branch applies against the `master` branch in
> git://ozlabs.org/~paulus/gitk.
>
> Michael
>
> Michael Haggerty (13):
> gitk: when processing tag labels, don't use `marks` as scratch space
> gitk: keep track of tag types in a separate `types` array
> gitk: use a type "tags" to indicate abbreviated tags
> gitk: use a type "mainhead" to indicate the main HEAD branch
> gitk: fill in `wvals` as the tags are first processed
> gitk: simplify regexp
> gitk: extract a method `remotereftext`
> gitk: only change the color of the "remote" part of remote refs
> gitk: shorten labels displayed for modern remote-tracking refs
> gitk: use type "remote" for remote-tracking references
> gitk: introduce a constant otherrefbgcolor
> gitk: add a configuration setting `remoterefbgcolor`
> gitk: change the default colors for remote-tracking references
>
> gitk | 114 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 76 insertions(+), 38 deletions(-)
>
^ 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
* 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: [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] 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: [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] 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: 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] 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] 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: 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
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