* Re: [PATCH 1/2] cherry-pick: demonstrate a segmentation fault
From: Johannes Schindelin @ 2016-11-26 12:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <89ffd6eaf4e1e121426c84f31dfc9c289f2a948b.1480091758.git.johannes.schindelin@gmx.de>
Hi,
On Fri, 25 Nov 2016, Johannes Schindelin wrote:
> +test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
Woops. This title is wrong. It should say instead: 'cherry-pick succeeds
with unrelated renamed, dirty file'.
> + test_commit to-rename &&
> + git checkout -b unrelated &&
> + test_commit unrelated &&
> + git checkout @{-1} &&
> + git mv to-rename.t renamed &&
> + test_tick &&
> + git commit -m renamed &&
> + echo modified >renamed &&
> + git cherry-pick unrelated
And this actually warns about an ambiguous refname.
Will send out v2 in a moment.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-11-26 12:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161125174721.f35mzc276kdwakzm@sigill.intra.peff.net>
Hi Peff,
On Fri, 25 Nov 2016, Jeff King wrote:
> On Fri, Nov 25, 2016 at 06:41:23PM +0100, Johannes Schindelin wrote:
>
> > > Ah, I didn't realize that was a requirement. If this is going to be part
> > > of a release and real end-users are going to see it, that does make me
> > > think the config option is the better path (than the presence of some
> > > file), as it's our standard way of tweaking run-time behavior.
> >
> > So how do you easily switch back and forth between testing the old vs the
> > new difftool via the test suite?
>
> If it's for a specific tool, I'd consider teaching the test suite to run
> the whole script twice: once with the flag set and once without.
>
> That is sometimes more complicated, though, if the script creates many
> sub-repos. An environment variable might be more natural. If you already
> support flipping the default via config, you can probably do:
>
> GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
> export GIT_CONFIG_PARAMETERS
Except that that does not work, of course. To figure out why, apply this
diff:
-- snip --
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 17f3008277..27159f65f3 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,6 +10,9 @@ Testing basic diff tool invocation
. ./test-lib.sh
+echo "config $(git config difftool.usebuiltin)." >&2
+exit 1
+
difftool_test_setup ()
{
test_config diff.tool test-tool &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9980a46133..0ddeded92b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -86,6 +86,7 @@ EDITOR=:
# /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets
# deriving from the command substitution clustered with the other
# ones.
+echo "before $(git config difftool.usebuiltin)." >&2
unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
my @env = keys %ENV;
my $ok = join("|", qw(
@@ -104,6 +105,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e
'
my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
print join("\n", @vars);
')
+echo "after $(git config difftool.usebuiltin)." >&2
unset XDG_CONFIG_HOME
unset GITPERLLIB
GIT_AUTHOR_EMAIL=author@example.com
-- snap --
and then weep at this output:
GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"; export GIT_CONFIG_PARAMETERS; bash t7800-difftool.sh -i -v -x
before true.
after .
Initialized empty Git repository in
/home/virtualbox/git/git-for-windows/t/trash
directory.t7800-difftool/.git/
config .
FATAL: Unexpected exit with code 1
In other words, GIT_CONFIG_PARAMETERS is *explicitly scrubbed* from the
environment when we run our tests (by the code block between the "before"
and the "after" statements in the diff above).
Ciao,
Dscho
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2016, #05; Wed, 23)
From: Duy Nguyen @ 2016-11-26 9:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqk2btlr3x.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 24, 2016 at 6:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> * nd/rebase-forget (2016-10-28) 1 commit
> - rebase: add --forget to cleanup rebase, leave HEAD untouched
>
> "git rebase" learned "--forget" option, which allows a user to
> remove the metadata left by an earlier "git rebase" that was
> manually aborted without using "git rebase --abort".
>
> Waiting for a reroll.
The reroll was http://public-inbox.org/git/%3C20161112020041.2335-1-pclouds@gmail.com%3E/
--
Duy
^ permalink raw reply
* Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin
From: Jakub Narębski @ 2016-11-25 21:24 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de>
W dniu 24.11.2016 o 21:55, Johannes Schindelin pisze:
> The current version of the builtin difftool does not, however, make full
> use of the internals but instead chooses to spawn a couple of Git
> processes, still, to make for an easier conversion. There remains a lot
> of room for improvement, left for a later date.
[...]
> Sadly, the speedup is more noticable on Linux than on Windows: a quick
> test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
> (real/user/sys) in a Linux VM, down from (6.529s/3.112s/0.644s), while
> on Windows, it is (36.064s/2.730s/7.194s), down from
> (47.637s/2.407s/6.863s). The culprit is most likely the overhead
> incurred from *still* having to shell out to mergetool-lib.sh and
> difftool--helper.sh.
Does this mean that our shell-based testsuite is not well suited to be
benchmark suite for comparing performance on MS Windows?
Or does it mean that "builtin-difftool" spawning Git processes is the
problem?
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/difftool.c | 670 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 669 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 53870bb..3480920 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -11,9 +11,608 @@
> *
> * Copyright (C) 2016 Johannes Schindelin
> */
> +#include "cache.h"
> #include "builtin.h"
> #include "run-command.h"
> #include "exec_cmd.h"
> +#include "parse-options.h"
> +#include "argv-array.h"
> +#include "strbuf.h"
> +#include "lockfile.h"
> +#include "dir.h"
> +
> +static char *diff_gui_tool;
> +static int trust_exit_code;
> +
> +static const char *const builtin_difftool_usage[] = {
> + N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
> + NULL
> +};
> +
> +static int difftool_config(const char *var, const char *value, void *cb)
> +{
> + if (!strcmp(var, "diff.guitool")) {
Shouldn't you also read other configuration variables, like "diff.tool",
and it's mergetool fallbacks ("merge.guitool", "merge.tool")?
> + diff_gui_tool = xstrdup(value);
> + return 0;
> + }
> +
> + if (!strcmp(var, "difftool.trustexitcode")) {
> + trust_exit_code = git_config_bool(var, value);
> + return 0;
> + }
Why you do not need to check "difftool.prompt"? And "mergetool.*" fallbacks?
> +
> + return git_default_config(var, value, cb);
> +}
> +
> +static int print_tool_help(void)
> +{
> + const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
> + return run_command_v_opt(argv, RUN_GIT_CMD);
This looks a bit strange to me, but I guess this is to avoid recursively
invoking ourself, and { "legacy-difftool", "--tool-help", NULL }; isn't
that much better.
> +}
> +
> +static int parse_index_info(char *p, int *mode1, int *mode2,
> + struct object_id *oid1, struct object_id *oid2,
> + char *status)
There are only two hard things in Computer Science:
cache invalidation and naming things.
-- Phil Karlton
Why did you name function that parses "diff --raw" output (aka "diff-tree"
output) parse_index_info() instead of parse_raw_diff() or parse_diff_tree()?
I went searching for `update-index --index-info` formats...
This is not that important, because this function is file-static; though
future developers would thank you for more descriptive naming.
ADDED: Disregard this, I see this function is about index (?) part of
raw diff, that is only a part of "git diff --raw -z" output. Though...
> +{
> + if (*p != ':')
> + return error("expected ':', got '%c'", *p);
> + *mode1 = (int)strtol(p + 1, &p, 8);
> + if (*p != ' ')
> + return error("expected ' ', got '%c'", *p);
Nitpicking.
I guess because this error shouldn't really happen, and because current
implementation is transient, we don't need to worry about better error
messages (was it problem with parsing, or was it unexpected character).
For example '10064x', or '10064\n' would fail parse, but it is not
space that we were expecting...
> + *mode2 = (int)strtol(p + 1, &p, 8);
> + if (*p != ' ')
> + return error("expected ' ', got '%c'", *p);
> + if (get_oid_hex(++p, oid1))
> + return error("expected object ID, got '%s'", p + 1);
> + p += GIT_SHA1_HEXSZ;
> + if (*p != ' ')
> + return error("expected ' ', got '%c'", *p);
> + if (get_oid_hex(++p, oid2))
> + return error("expected object ID, got '%s'", p + 1);
> + p += GIT_SHA1_HEXSZ;
> + if (*p != ' ')
> + return error("expected ' ', got '%c'", *p);
> + *status = *++p;
> + if (!status || p[1])
> + return error("unexpected trailer: '%s'", p);
> + return 0;
> +}
> +
> +/*
> + * Remove any trailing slash from $workdir
> + * before starting to avoid double slashes in symlink targets.
> + */
Err... that's not what add_path() does, in its current implementation.
It doesn't remove trailing slashes, but it checks if there is trailing
slash, and if there isn't, it adds it as separator before adding path.
Or was it original comment from the Perl implementation? It look
like this, with '$workdir'... If it is meant to be straight copy
of comment from legacy-difftool, a note would be nice.
> +static void add_path(struct strbuf *buf, size_t base_len, const char *path)
Naming: I think strbuf_addpath() would be a better name, but I guess
it is a matter of taste.
> +{
> + strbuf_setlen(buf, base_len);
> + if (buf->len && buf->buf[buf->len - 1] != '/')
> + strbuf_addch(buf, '/');
> + strbuf_addstr(buf, path);
> +}
> +
> +/*
> + * Determine whether we can simply reuse the file in the worktree.
> + */
> +static int use_wt_file(const char *workdir, const char *name,
Should it be 'name' or 'pathname'?
> + struct object_id *oid)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct stat st;
> + int use = 0;
> +
> + strbuf_addstr(&buf, workdir);
> + add_path(&buf, buf.len, name);
With proposed rename, it would IMVVVHO looks better
+ strbuf_addstr(&buf, workdir);
+ strbuf_addpath(&buf, buf.len, name);
But that is a matter of taste (again, the function is file-local).
> +
> + if (!lstat(buf.buf, &st) && !S_ISLNK(st.st_mode)) {
> + struct object_id wt_oid;
> + int fd = open(buf.buf, O_RDONLY);
> +
> + if (!index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
> + if (is_null_oid(oid)) {
> + oidcpy(oid, &wt_oid);
> + use = 1;
> + } else if (!oidcmp(oid, &wt_oid))
> + use = 1;
> + }
> + }
> +
> + strbuf_release(&buf);
> +
> + return use;
> +}
[...]
> +static int ensure_leading_directories(char *path)
> +{
> + switch (safe_create_leading_directories(path)) {
> + case SCLD_OK:
> + case SCLD_EXISTS:
> + return 0;
> + default:
> + return error(_("could not create leading directories "
> + "of '%s'"), path);
> + }
> +}
Nice function, I wonder if it would be useful in other places.
> +
> +static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> + int argc, const char **argv)
> +{
> + char tmpdir[PATH_MAX];
> + struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> + struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
Nitpicking.
To be symmetric, it could be reordered like this:
+ struct strbuf info = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf lpath = STRBUF_INIT, rpath = STRBUF_INIT;
See: lpath, rpath; ldir, rdir; ldir_len, rdir_len.
> + struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> + struct strbuf wtdir = STRBUF_INIT;
> + size_t ldir_len, rdir_len, wtdir_len;
[...]
> + argv_array_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
> + NULL);
> + for (i = 0; i < argc; i++)
> + argv_array_push(&child.args, argv[i]);
> + if (start_command(&child))
> + die("could not obtain raw diff");
> + fp = xfdopen(child.out, "r");
> +
> + /* Build index info for left and right sides of the diff */
> + i = 0;
> + while (!strbuf_getline_nul(&info, fp)) {
> + int lmode, rmode;
> + struct object_id loid, roid;
> + char status;
> + const char *src_path, *dst_path;
> + size_t src_path_len, dst_path_len;
> +
> + if (starts_with(info.buf, "::"))
> + die(N_("combined diff formats('-c' and '--cc') are "
> + "not supported in\n"
> + "directory diff mode('-d' and '--dir-diff')."));
> +
> + if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,
> + &status))
After rename it would read as:
+ if (parse_raw_diff(info.buf, &lmode, &rmode, &loid, &roid,
+ &status))
Though now I see that you parse here index information of raw diff
(I think)... so disregard my musings.
> + break;
> + if (strbuf_getline_nul(&lpath, fp))
> + break;
> + src_path = lpath.buf;
> + src_path_len = lpath.len;
> +
> + i++;
> + if (status != 'C' && status != 'R') {
> + dst_path = src_path;
> + dst_path_len = src_path_len;
> + } else {
> + if (strbuf_getline_nul(&rpath, fp))
> + break;
> + dst_path = rpath.buf;
> + dst_path_len = rpath.len;
> + }
[...]
> + /*
> + * Changes to submodules require special treatment.This loop writes a
Here and in few other places you are missing space after full stop.
+ * Changes to submodules require special treatment. This loop writes a
> + * temporary file to both the left and right directories to show the
> + * change in the recorded SHA1 for the submodule.
> + */
> + hashmap_iter_init(&submodules, &iter);
> + while ((entry = hashmap_iter_next(&iter))) {
> + if (*entry->left) {
> + add_path(&ldir, ldir_len, entry->path);
> + ensure_leading_directories(ldir.buf);
> + write_file(ldir.buf, "%s", entry->left);
> + }
> + if (*entry->right) {
> + add_path(&rdir, rdir_len, entry->path);
> + ensure_leading_directories(rdir.buf);
> + write_file(rdir.buf, "%s", entry->right);
> + }
> + }
> +
> + /*
> + * Symbolic links require special treatment.The standard "git diff"
Same here.
> + * shows only the link itself, not the contents of the link target.
> + * This loop replicates that behavior.
> + */
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [char-misc-next] mei: request async autosuspend at the end of enumeration
From: Jakub Narębski @ 2016-11-25 20:33 UTC (permalink / raw)
To: Jeff King, Winkler, Tomas
Cc: Matthieu Moy, git@vger.kernel.org,
Greg KH (gregkh@linuxfoundation.org), Usyskin, Alexander,
linux-kernel@vger.kernel.org
In-Reply-To: <20161125031425.gefijvssvygp6pl4@sigill.intra.peff.net>
W dniu 25.11.2016 o 04:14, Jeff King pisze:
> On Thu, Nov 24, 2016 at 10:37:14PM +0000, Winkler, Tomas wrote:
>
>>>>> Cc: <stable@vger.kernel.org> # 4.4+
>>>>
>>>> Looks like git send-email is not able to parse this address correctly
>>>> though this is suggested format by Documentation/stable_kernel_rules.txt.
>>>> Create wrong address If git parsers is used : 'stable@vger.kernel.org#4.4+'
[...]
> The patch just brings parity to the Mail::Address behavior and git's
> fallback parser, so that you don't end up with the broken
> stable@vger.kernel.org#4.4+ address. Instead, that content goes into the
> name part of the address.
>
> It sounds like you want the "# 4.4+" to be dropped entirely in the
> rfc822 header. It looks like send-email used to do that, but stopped in
> b1c8a11c8 (send-email: allow multiple emails using --cc, --to and --bcc,
> 2015-06-30).
>
> So perhaps there are further fixes required, but it's hard to know. The
> input isn't a valid rfc822 header, so it's not entirely clear what the
> output is supposed to be. I can buy either "drop it completely" or
> "stick it in the name field of the cc header" as reasonable.
Well, we could always convert it to email address comment, converting
for example the following trailer:
Cc: John Doe <john@example.com> # comment
to the following address:
John Doe <john@example.com> (comment)
Just FYI. Though I'm not sure how well this would work...
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #05; Wed, 23)
From: Jakub Narębski @ 2016-11-25 19:40 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqk2btlr3x.fsf@gitster.mtv.corp.google.com>
W dniu 24.11.2016 o 00:21, Junio C Hamano pisze:
> * nd/rebase-forget (2016-10-28) 1 commit
> - rebase: add --forget to cleanup rebase, leave HEAD untouched
>
> "git rebase" learned "--forget" option, which allows a user to
> remove the metadata left by an earlier "git rebase" that was
> manually aborted without using "git rebase --abort".
>
> Waiting for a reroll.
It's always a good thing to stop requiring messing with .git insides.
> * jc/reset-unmerge (2016-10-24) 1 commit
> - reset: --unmerge
>
> After "git add" is run prematurely during a conflict resolution,
> "git diff" can no longer be used as a way to sanity check by
> looking at the combined diff. "git reset" learned a new
> "--unmerge" option to recover from this situation.
>
> Will discard.
> This may not be needed, given that update-index has a similar
> option.
OTOH update-index is considered plumbing, so having "git reset --unmerge"
might be good thing (note that we can re-checkout file merge).
> * jc/merge-drop-old-syntax (2015-04-29) 1 commit
> (merged to 'next' on 2016-10-11 at 8928c8b9b3)
> + merge: drop 'git merge <message> HEAD <commit>' syntax
>
> Stop supporting "git merge <message> HEAD <commit>" syntax that has
> been deprecated since October 2007, and issues a deprecation
> warning message since v2.5.0.
>
> Will cook in 'next'.
>
^ permalink raw reply
* [feature request] Make "commit --only" work with new files
From: Luis Ressel @ 2016-11-25 16:56 UTC (permalink / raw)
To: git
Hello,
currently "git commit --only <file>" only works if <file> is already
checked into the repo, but not with newly created and still untracked
files (builtin/commit.c:list_path() throws the error "error: pathspec
'<file>' did not match any file(s) known to git.")
I don't think this limitation is intented. I've had a look at the
relevant part of builtin/commit.c, but unfortunately it wasn't obvious
to me how to fix this.
Regards,
Luis Ressel
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-11-25 17:47 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611251841030.117539@virtualbox>
On Fri, Nov 25, 2016 at 06:41:23PM +0100, Johannes Schindelin wrote:
> > Ah, I didn't realize that was a requirement. If this is going to be part
> > of a release and real end-users are going to see it, that does make me
> > think the config option is the better path (than the presence of some
> > file), as it's our standard way of tweaking run-time behavior.
>
> So how do you easily switch back and forth between testing the old vs the
> new difftool via the test suite?
If it's for a specific tool, I'd consider teaching the test suite to run
the whole script twice: once with the flag set and once without.
That is sometimes more complicated, though, if the script creates many
sub-repos. An environment variable might be more natural. If you already
support flipping the default via config, you can probably do:
GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
export GIT_CONFIG_PARAMETERS
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-11-25 17:41 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161125171940.rizbqyhsygdsoujr@sigill.intra.peff.net>
Hi Peff,
On Fri, 25 Nov 2016, Jeff King wrote:
> On Fri, Nov 25, 2016 at 12:05:00PM +0100, Johannes Schindelin wrote:
>
> > > I would have expected it to just be a build-time flag, like:
> > >
> > > make BUILTIN_DIFFTOOL=Yes test
> >
> > That works for Git developers.
> >
> > I want to let as many users as possible test the builtin difftool.
> > Hopefully a lot more users than there are Git developers.
> >
> > Which means that I need a feature flag in production code, not a build
> > time flag.
>
> Ah, I didn't realize that was a requirement. If this is going to be part
> of a release and real end-users are going to see it, that does make me
> think the config option is the better path (than the presence of some
> file), as it's our standard way of tweaking run-time behavior.
So how do you easily switch back and forth between testing the old vs the
new difftool via the test suite?
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2] date-formats.txt: Typo fix
From: Luis Ressel @ 2016-11-25 17:36 UTC (permalink / raw)
To: git
Last time I checked, I was living in the UTC+01:00 time zone. UTC+02:00
would be Central European _Summer_ Time.
---
Documentation/date-formats.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt
index 35e8da2..6926e0a 100644
--- a/Documentation/date-formats.txt
+++ b/Documentation/date-formats.txt
@@ -11,7 +11,7 @@ Git internal format::
It is `<unix timestamp> <time zone offset>`, where `<unix
timestamp>` is the number of seconds since the UNIX epoch.
`<time zone offset>` is a positive or negative offset from UTC.
- For example CET (which is 2 hours ahead UTC) is `+0200`.
+ For example CET (which is 1 hour ahead of UTC) is `+0100`.
RFC 2822::
The standard email format as described by RFC 2822, for example
--
2.10.2
^ permalink raw reply related
* [PATCH] date-formats.txt: Typo fix
From: Luis Ressel @ 2016-11-25 17:34 UTC (permalink / raw)
To: git
Last time I checked, I was living in the UTC+01:00 time zone. UTC+02:00
would be Central European _Summer_ Time.
---
Documentation/date-formats.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt
index 35e8da2..9c3084e 100644
--- a/Documentation/date-formats.txt
+++ b/Documentation/date-formats.txt
@@ -11,7 +11,7 @@ Git internal format::
It is `<unix timestamp> <time zone offset>`, where `<unix
timestamp>` is the number of seconds since the UNIX epoch.
`<time zone offset>` is a positive or negative offset from UTC.
- For example CET (which is 2 hours ahead UTC) is `+0200`.
+ For example CET (which is 1 hours ahead of UTC) is `+0100`.
RFC 2822::
The standard email format as described by RFC 2822, for example
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v2] merge-recursive.c: use string_list_sort instead of qsort
From: Jeff King @ 2016-11-25 17:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, René Scharfe, Junio C Hamano
In-Reply-To: <CACsJy8Atv9rkwmCcXgOqDb6pLP8RxQ7XnxMYt3=hN6KG4X79iA@mail.gmail.com>
On Fri, Nov 25, 2016 at 07:15:15PM +0700, Duy Nguyen wrote:
> > I guess I haven't used string_list_sort() in a while, but I was
> > surprised to find that it just feeds the strings to the comparator. That
> > makes sense for using a raw strcmp() as the comparator, but I wonder if
> > any callers would ever want to take the util field into account (e.g.,
> > to break ties).
> >
> > We don't seem to care here, though (which can be verified by reading the
> > code, but also because any mention of one->util would be a compilation
> > error after your patch). So I guess we can punt on it until the day that
> > some caller does need it.
>
> Some callers do need it, or at least fmt-merge-msg.c:add_people_info()
> does, maybe builtin/remote.c:show() and shortlog.c:shortlog_output()
> too. But I'll stop here and get back to my worktree stuff.
I started to work on this, figuring it would be a nice warm-up for the
day. But it actually is a little complicated, and I think not worth
doing. :)
The obvious backwards-compatible way to do it is to add a "cmp_item"
field to the string list. Sorting should use that if non-NULL, and
fallback to the string-oriented "cmp" otherwise.
And that does work when you want to sort via string_list_sort, like:
authors->cmp_item = cmp_string_list_util_as_integral;
string_list_sort(authors);
(the example is from fmt-merge-message.c). But the original use of
sorting in string-list was to keep a sorted list as you go with
string_list_insert(). And in that call we have _only_ the newly added
string, and the caller has not yet had an opportunity to set the util
field. So:
struct string_list list = STRING_LIST_INIT_DUP;
list.cmp_item = cmp_util_fields;
for (...)
string_list_insert(&list, foo[i])->util = bar[i];
is nonsense. It would always see a NULL util field during the
comparison.
Certainly "don't do that" is a possible answer. But it's just a bad
interface. It encourages a nonsensical use, and it makes a natural use
(sorting after the fact) more clunky by making the caller set a field in
the struct rather than pass a parameter. The correct interface is more
like:
string_list_sort_items(authors, cmp_string_list_util_as_integral);
but then we are not really saving much over the more generic:
QSORT(authors->items, authors->nr, cmp_string_list_util_as_integral);
So I'm inclined to leave it as-is.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-11-25 17:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611251201580.117539@virtualbox>
On Fri, Nov 25, 2016 at 12:05:00PM +0100, Johannes Schindelin wrote:
> > I would have expected it to just be a build-time flag, like:
> >
> > make BUILTIN_DIFFTOOL=Yes test
>
> That works for Git developers.
>
> I want to let as many users as possible test the builtin difftool.
> Hopefully a lot more users than there are Git developers.
>
> Which means that I need a feature flag in production code, not a build
> time flag.
Ah, I didn't realize that was a requirement. If this is going to be part
of a release and real end-users are going to see it, that does make me
think the config option is the better path (than the presence of some
file), as it's our standard way of tweaking run-time behavior.
The implementation can still remain slightly gross if it's eventually
going away.
> > I'm happy with pretty much anything under the reasoning of "this does not
> > matter much because it is going away soon".
>
> Yeah, well, I am more happy with anything along the lines of David's
> review, pointing out flaws in the current revision of the builtin difftool
> before it bites users ;-)
Sorry, I can't really help much there, not having much knowledge of
difftool.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] Fix segmentation fault with cherry-pick
From: Johannes Schindelin @ 2016-11-25 16:41 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480091758.git.johannes.schindelin@gmx.de>
Hi,
On Fri, 25 Nov 2016, Johannes Schindelin wrote:
> The culprit is actually not cherry-pick, but a special code path that
> expects refresh_cache_entry() not to return NULL. And the fix is to
> teach it to handle NULL there.
>
> This bug was brought to my attention by Markus Klein via
> https://github.com/git-for-windows/git/issues/952.
For the record, I looked at other callers of `refresh_cache_entry()`:
there is only `make_cache_entry()`, whose callers all handle NULL return
values except in resolve-undo.c. But that latter caller is okay because it
specifically does not allow refreshing (by passing 0 as options), so
refresh_cache_entry() cannot return NULL.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 2/2] Avoid a segmentation fault with renaming merges
From: Johannes Schindelin @ 2016-11-25 16:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480091758.git.johannes.schindelin@gmx.de>
Under very particular circumstances, merge-recursive's `add_cacheinfo()`
function gets a `NULL` returned from `refresh_cache_entry()` without
expecting it, and subsequently passes it to `add_cache_entry()` which
consequently crashes.
Let's not crash.
This fixes https://github.com/git-for-windows/git/issues/952
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-recursive.c | 2 ++
t/t3501-revert-cherry-pick.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..609061f58a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
struct cache_entry *nce;
nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+ if (!nce)
+ return err(o, _("addinfo: '%s' is not up-to-date"), path);
if (nce != ce)
ret = add_cache_entry(nce, options);
}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index d96d391af3..8e21840f11 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
'
-test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
+test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
test_commit to-rename &&
git checkout -b unrelated &&
test_commit unrelated &&
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 1/2] cherry-pick: demonstrate a segmentation fault
From: Johannes Schindelin @ 2016-11-25 16:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480091758.git.johannes.schindelin@gmx.de>
In https://github.com/git-for-windows/git/issues/952, a complicated
scenario was described that leads to a segmentation fault in
cherry-pick.
It boils down to a certain code path involving a renamed file that is
dirty, for which `refresh_cache_entry()` returns `NULL`, and that
`NULL` not being handled properly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 394f0005a1..d96d391af3 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,4 +141,16 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
'
+test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
+ test_commit to-rename &&
+ git checkout -b unrelated &&
+ test_commit unrelated &&
+ git checkout @{-1} &&
+ git mv to-rename.t renamed &&
+ test_tick &&
+ git commit -m renamed &&
+ echo modified >renamed &&
+ git cherry-pick unrelated
+'
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 0/2] Fix segmentation fault with cherry-pick
From: Johannes Schindelin @ 2016-11-25 16:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
The culprit is actually not cherry-pick, but a special code path that
expects refresh_cache_entry() not to return NULL. And the fix is to
teach it to handle NULL there.
This bug was brought to my attention by Markus Klein via
https://github.com/git-for-windows/git/issues/952.
Johannes Schindelin (2):
cherry-pick: demonstrate a segmentation fault
Avoid a segmentation fault with renaming merges
merge-recursive.c | 2 ++
t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
2 files changed, 14 insertions(+)
base-commit: e2b2d6a172b76d44cb7b1ddb12ea5bfac9613a44
Published-As: https://github.com/dscho/git/releases/tag/cherry-pick-segfault-v1
Fetch-It-Via: git fetch https://github.com/dscho/git cherry-pick-segfault-v1
--
2.11.0.rc3.windows.1
^ permalink raw reply
* Re: [PATCH v8 07/10] convert: unify the "auto" handling of CRLF
From: Torsten Bögershausen @ 2016-11-25 15:48 UTC (permalink / raw)
To: tboegi, git
In-Reply-To: <1461942123-16179-1-git-send-email-tboegi@web.de>
RFH, the normalization as descrived in Documentation/gitattributes.txt
does not work anymore:
From a clean working directory:
-------------------------------------------------
$ echo "* text=auto" >.gitattributes
$ rm .git/index # Remove the index to force Git to
$ git reset # re-scan the working directory
$ git status # Show files that will be normalized
$ git add -u
$ git add .gitattributes
$ git commit -m "Introduce end-of-line normalization"
-------------------------------------------------
I have different ideas, how a a normalizatio can be done:
A)
-------------------------------------------------
$ echo "* text=auto" >.gitattributes
$ rm .git/index
$ git add .
$ git status # Show files that will be normalized
$ git commit -m "Introduce end-of-line normalization"
-------------------------------------------------
B)
$ echo "* text=auto" >.gitattributes &&
$ git add .gitattributes &&
$ git ls-files --eol | egrep '^i/(crlf|mixed).*attr/(text|auto)' | ( TAB=$(printf "\t") ; sed -e "s/.*$TAB/dos2unix /" ) >/tmp/$$ &&
$ /bin/sh /tmp/$$ &&
$ rm -f /tmp/$$ &&
$ git add -u &&
$ git commit -m "Introduce end-of-line normalization"
C)
Teach "git add" to learn --renormalize and then
-------------------------------------------------
$ echo "* text=auto" >.gitattributes
$ git add -u --renormalize
$ git add .gitattributes
$ git commit -m "Introduce end-of-line normalization"
-------------------------------------------------
(None of them is really tested)
A) may loose the execute bit
B) dos2unix is not installed everywhere (like Mac OS)
C) seems to most attractive, but I couldn't find out how to forward
options from "git add" into convert.c
Any help is appreciated.
^ permalink raw reply
* Re: [PATCH 0/3] Minor fixes on 'git worktree'
From: Duy Nguyen @ 2016-11-25 13:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Mike Rappazzo
In-Reply-To: <CACsJy8D+vyO0LF_A2Wym+PUg=HOTPyDe-1fVwv8zN797pL-rmw@mail.gmail.com>
On Fri, Nov 25, 2016 at 7:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> Adding the test for the failed parse_ref() is possible, I think. But
> since that function is destined to die, as I promised to use
> refs-provided api instead of rolling out a custom ref parser, and I'm
> going to have another hard look at refs subsystem for the
> gc-not-looking-at-worktree-refs problem, may I make another promise to
> add tests after this function is gone?
Never mind. I went ahead and made a test case anyway because why not.
But that's not worth an email. An interesting thing maybe worth
sharing is, if HEAD is broken (the only reason we would fail to create
"struct worktree" for main worktree), then "git worktree list" from
main worktree would fail too, because repo setup code fails to parse
HEAD as well and die(). That makes the "always show main worktree"
patch useless, since we won't get far enough to execute
get_worktrees() in the first place. However, you can still do "git
worktree list" from a linked worktree, gitdir setup code will not stop
you (how can it). And the patch is still needed.
--
Duy
^ permalink raw reply
* Re: [PATCH 0/3] Minor fixes on 'git worktree'
From: Duy Nguyen @ 2016-11-25 12:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Mike Rappazzo
In-Reply-To: <xmqqbmx6nnpg.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 23, 2016 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> This fixes two things:
>>
>> - make sure the first item is always the main worktree even if we
>> fail to retrieve some info
>>
>> - keep 'worktree list' order stable (which in turn fixes the random
>> failure on my 'worktree-move' series
>> Nguyễn Thái Ngọc Duy (3):
>> worktree.c: zero new 'struct worktree' on allocation
>> get_worktrees() must return main worktree as first item even on error
>> worktree list: keep the list sorted
>>
>> builtin/worktree.c | 26 ++++++++++++++++++++++----
>> worktree.c | 20 ++++----------------
>> 2 files changed, 26 insertions(+), 20 deletions(-)
>
> Any tests?
I discarded the idea of adding test for sorting because it relies on
filesystems. A passed test may just mean the filesystem happens to
return files in "good" order. But I guess a test wouldn't hurt.
Somewhere out there some user may still have a "bad" filesystem and
the test could help catch breakage then.
Adding the test for the failed parse_ref() is possible, I think. But
since that function is destined to die, as I promised to use
refs-provided api instead of rolling out a custom ref parser, and I'm
going to have another hard look at refs subsystem for the
gc-not-looking-at-worktree-refs problem, may I make another promise to
add tests after this function is gone? It should happen "soon".
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] worktree list: keep the list sorted
From: Duy Nguyen @ 2016-11-25 12:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Mike Rappazzo
In-Reply-To: <xmqqwpfum801.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 24, 2016 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> More importantly, perhaps get_worktrees() should learn to take an
> optional pointer to int that returns how many items are in the list?
My first thought was "yeah I remember there are many counting loop
like this" then grepped and realized this is the only place. For now I
think it's ok to leave it as is. When the second place does the same,
we'll change get_worktrees()
> Alternatively, other existing callers of the function do not care
> about the order, so it may not be such a good idea to always sort
> the result, but it may be a good idea to teach it to take a boolean
> that signals that the list should be sorted in a "natural order",
> which is how "worktree list" would show them to the user?
OK. I'll make it a bit flag, not a boolean though, to avoid changing
all call sites next time.
--
Duy
^ permalink raw reply
* Re: [PATCH v2] merge-recursive.c: use string_list_sort instead of qsort
From: Duy Nguyen @ 2016-11-25 12:15 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, René Scharfe, Junio C Hamano
In-Reply-To: <20161124205242.ts4qhvfz2hvxez36@sigill.intra.peff.net>
On Fri, Nov 25, 2016 at 3:52 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 24, 2016 at 06:45:36PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This started out to as a hunt for remaining qsort() calls after rs/qsort
>> series because qsort() API is a bit easy to get wrong (*). However,
>> since we have string_list_sort(), it's conceptually a better way to sort
>> here.
>>
>> (*) In this particular case, it's even more confusing when you sort one
>> variable but you use the number of items and item size from an unrelated
>> variable (from a first glance)
>
> Makes sense, though I think I probably would have explained it in
> reverse order:
>
> Merge-recursive sorts a string list using a raw qsort(), where it
> feeds the "items" from one struct but the "nr" and size fields from
> another struct. This isn't a bug because one list is a copy of the
> other, but it's unnecessarily confusing (and also caused our recent
> QSORT() cleanups via coccinelle to miss this call site).
>
> Let's use string_list_sort() instead, which is more concise and harder
> to get wrong. Note that we need to adjust our comparison function,
> which gets fed only the strings now, not the string_list_items. That's
> OK because we don't use the "util" field as part of our sort.
>
> Feel free to use or ignore my description as you see fit. :)
I delegate the decision to Junio. He can amend the commit if he
decides so. I suspect it's a good idea to do so.
>> -static int string_list_df_name_compare(const void *a, const void *b)
>> +static int string_list_df_name_compare(const char *one, const char *two)
>> {
>> - const struct string_list_item *one = a;
>> - const struct string_list_item *two = b;
>> - int onelen = strlen(one->string);
>> - int twolen = strlen(two->string);
>> + int onelen = strlen(one);
>> + int twolen = strlen(two);
>
> I guess I haven't used string_list_sort() in a while, but I was
> surprised to find that it just feeds the strings to the comparator. That
> makes sense for using a raw strcmp() as the comparator, but I wonder if
> any callers would ever want to take the util field into account (e.g.,
> to break ties).
>
> We don't seem to care here, though (which can be verified by reading the
> code, but also because any mention of one->util would be a compilation
> error after your patch). So I guess we can punt on it until the day that
> some caller does need it.
Some callers do need it, or at least fmt-merge-msg.c:add_people_info()
does, maybe builtin/remote.c:show() and shortlog.c:shortlog_output()
too. But I'll stop here and get back to my worktree stuff.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-11-25 11:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161125031809.mueurvjijlnzy2s5@sigill.intra.peff.net>
Hi Peff,
On Thu, 24 Nov 2016, Jeff King wrote:
> On Thu, Nov 24, 2016 at 10:56:23PM +0100, Johannes Schindelin wrote:
>
> > > I think it would probably be OK to ship with that caveat (people would
> > > probably use --global config, or "git -c" for a quick override), but if
> > > you really wanted to address it, you can do something like what
> > > pager.c:read_early_config() does.
> >
> > The config setting is already overkill (and does even make something much
> > harder than before: running tests with the builtin difftool used to be as
> > simply as `touch use-builtin-difftool && make -C t t7800-difftool.sh, now
> > I have to edit t7800-difftool.sh to configure difftool.useBuiltin, and
> > without the repo-level config even that would not be working).
> >
> > Imitating read_early_config() would be overkill deluxe.
>
> I would have expected it to just be a build-time flag, like:
>
> make BUILTIN_DIFFTOOL=Yes test
That works for Git developers.
I want to let as many users as possible test the builtin difftool.
Hopefully a lot more users than there are Git developers.
Which means that I need a feature flag in production code, not a build
time flag.
> I'm happy with pretty much anything under the reasoning of "this does not
> matter much because it is going away soon".
Yeah, well, I am more happy with anything along the lines of David's
review, pointing out flaws in the current revision of the builtin difftool
before it bites users ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-11-25 3:18 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611242211450.117539@virtualbox>
On Thu, Nov 24, 2016 at 10:56:23PM +0100, Johannes Schindelin wrote:
> > I think it would probably be OK to ship with that caveat (people would
> > probably use --global config, or "git -c" for a quick override), but if
> > you really wanted to address it, you can do something like what
> > pager.c:read_early_config() does.
>
> The config setting is already overkill (and does even make something much
> harder than before: running tests with the builtin difftool used to be as
> simply as `touch use-builtin-difftool && make -C t t7800-difftool.sh, now
> I have to edit t7800-difftool.sh to configure difftool.useBuiltin, and
> without the repo-level config even that would not be working).
>
> Imitating read_early_config() would be overkill deluxe.
I would have expected it to just be a build-time flag, like:
make BUILTIN_DIFFTOOL=Yes test
but I did not closely follow the rest of the conversation, so I am
probably just repeating bits that were already said. So probably ignore
me.
I'm happy with pretty much anything under the reasoning of "this does not
matter much because it is going away soon".
-Peff
^ permalink raw reply
* Re: [char-misc-next] mei: request async autosuspend at the end of enumeration
From: Jeff King @ 2016-11-25 3:14 UTC (permalink / raw)
To: Winkler, Tomas
Cc: Matthieu Moy, git@vger.kernel.org,
Greg KH (gregkh@linuxfoundation.org), Usyskin, Alexander,
linux-kernel@vger.kernel.org
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B54331476@hasmsx108.ger.corp.intel.com>
On Thu, Nov 24, 2016 at 10:37:14PM +0000, Winkler, Tomas wrote:
> > > > Cc: <stable@vger.kernel.org> # 4.4+
> > >
> > > Looks like git send-email is not able to parse this address correctly
> > > though this is suggested format by Documentation/stable_kernel_rules.txt.
> > > Create wrong address If git parsers is used : 'stable@vger.kernel.org#4.4+'
> > >
> > > Something like s/#.*$// is needed before parsing Cc:
> >
> > This should be fixed by e3fdbcc8e (parse_mailboxes: accept extra text after
> > <...> address, 2016-10-13), which will be released next week as part of v2.11. As
> > a workaround, you can also install the Mail::Address perl module. See [1] for
> > the gory details.
>
> Thanks for update, I failed to understand from the thread though what
> decision was actually applied, I will look at the patch itself. I've
> tried to install Mail::Address and it fixes the actual address, but it
> appends the # 4.4+ suffix into the name as also mentioned in the
> thread, which doesn't fit the stable rules doc.
The patch just brings parity to the Mail::Address behavior and git's
fallback parser, so that you don't end up with the broken
stable@vger.kernel.org#4.4+ address. Instead, that content goes into the
name part of the address.
It sounds like you want the "# 4.4+" to be dropped entirely in the
rfc822 header. It looks like send-email used to do that, but stopped in
b1c8a11c8 (send-email: allow multiple emails using --cc, --to and --bcc,
2015-06-30).
So perhaps there are further fixes required, but it's hard to know. The
input isn't a valid rfc822 header, so it's not entirely clear what the
output is supposed to be. I can buy either "drop it completely" or
"stick it in the name field of the cc header" as reasonable.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox