* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Duy Nguyen @ 2017-01-19 12:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqvatouwsh.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> So what should we do if freshen_file() returns 0 which means that the
>>>> freshening failed?
>>>
>>> You tell me ;-) as you are the one who is proposing this feature.
>>
>> My answer is, we are not worse than freshening loose objects case
>> (especially since I took the idea from there).
>
> I do not think so, unfortunately. Loose object files with stale
> timestamps are not removed as long as objects are still reachable.
But there are plenty of unreachable loose objects, added in index,
then got replaced with new versions. cache-tree can create loose trees
too and it's been run more often, behind user's back, to take
advantage of the shortcut in unpack-trees.
> For the base/shared index file, the timestamp is the only thing that
> protects them from pruning, unless it is serving as the base file
> for the currently active $GIT_DIR/index that is split.
--
Duy
^ permalink raw reply
* [PATCH] log: new option decorate reflog of remote refs
From: Nguyễn Thái Ngọc Duy @ 2017-01-19 12:26 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This is most useful when you fork your branches off a remote ref and
rely on ref decoration to show your fork points in `git log`. Then you
do a "git fetch" and suddenly the remote decoration is gone because
remote refs are moved forward. With this, we can still see something
like "origin/foo@{1}"
This is for remote refs only because based on my experience, docorating
local reflog is just too noisy. You will most likely see HEAD@{1},
HEAD@{2} and so on. We can add that as a separate option in future if we
see a need for it.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
I've been using this for many weeks and it has proven its usefulness
(to me). Looks like good material to send upstream.
Documentation/git-log.txt | 5 +++++
builtin/log.c | 10 +++++++++-
log-tree.c | 43 +++++++++++++++++++++++++++++++++++++++----
log-tree.h | 2 +-
pretty.c | 4 ++--
revision.c | 2 +-
6 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 32246fd..f5ee575 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -38,6 +38,11 @@ OPTIONS
are shown as if 'short' were given, otherwise no ref names are
shown. The default option is 'short'.
+--decorate-remote-reflog[=<n>]::
+ Decorate `<n>` most recent reflog entries on remote refs, up
+ to the specified number of entries. By default, only the most
+ recent reflog entry is decorated.
+
--source::
Print out the ref name given on the command line by which each
commit was reached.
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..c208703 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int default_follow;
static int default_show_signature;
static int decoration_style;
static int decoration_given;
+static int decorate_remote_reflog;
static int use_mailmap_config;
static const char *fmt_patch_subject_prefix = "PATCH";
static const char *fmt_pretty;
@@ -141,6 +142,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate options"),
PARSE_OPT_OPTARG, decorate_callback},
+ { OPTION_INTEGER, 0, "decorate-remote-reflog",
+ &decorate_remote_reflog, N_("n"),
+ N_("decorate the last <n> reflog entries of remote refs"),
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, NULL, 1 },
OPT_CALLBACK('L', NULL, &line_cb, "n,m:file",
N_("Process line range n,m in file, counting from 1"),
log_line_range_callback),
@@ -195,9 +200,12 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
rev->abbrev_commit = 0;
}
+ if (decorate_remote_reflog > 0 && !decoration_style)
+ decoration_style = DECORATE_SHORT_REFS;
if (decoration_style) {
rev->show_decorations = 1;
- load_ref_decorations(decoration_style);
+ load_ref_decorations(decoration_style,
+ decorate_remote_reflog);
}
if (rev->line_level_traverse)
diff --git a/log-tree.c b/log-tree.c
index 8c24157..3d85ebc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -88,14 +88,37 @@ const struct name_decoration *get_name_decoration(const struct object *obj)
return lookup_decoration(&name_decoration, obj);
}
+struct reflog_cb {
+ int type;
+ int count;
+ int nth;
+ const char *refname;
+};
+
+static int add_nth_reflog(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
+{
+ struct reflog_cb *cb = cb_data;
+ struct commit *commit;
+
+ commit = lookup_commit(nsha1);
+ if (commit && cb->nth) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s@{%d}", cb->refname, cb->nth);
+ add_name_decoration(cb->type, sb.buf, &commit->object);
+ strbuf_release(&sb);
+ }
+ cb->nth++;
+ return cb->nth >= cb->count;
+}
+
static int add_ref_decoration(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
struct object *obj;
enum decoration_type type = DECORATION_NONE;
- assert(cb_data == NULL);
-
if (starts_with(refname, git_replace_ref_base)) {
struct object_id original_oid;
if (!check_replace_refs)
@@ -135,6 +158,17 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
parse_object(obj->oid.hash);
add_name_decoration(DECORATION_REF_TAG, refname, obj);
}
+
+ if (cb_data && type == DECORATION_REF_REMOTE) {
+ struct reflog_cb cb;
+
+ memset(&cb, 0, sizeof(cb));
+ cb.refname = refname;
+ cb.type = type;
+ cb.count = *(int *)cb_data + 1 /* for @{0} */;
+
+ for_each_reflog_ent_reverse(refname, add_nth_reflog, &cb);
+ }
return 0;
}
@@ -147,13 +181,14 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)
return 0;
}
-void load_ref_decorations(int flags)
+void load_ref_decorations(int flags, int remote_reflog)
{
if (!decoration_loaded) {
+ void *cb = remote_reflog ? &remote_reflog : NULL;
decoration_loaded = 1;
decoration_flags = flags;
- for_each_ref(add_ref_decoration, NULL);
+ for_each_ref(add_ref_decoration, cb);
head_ref(add_ref_decoration, NULL);
for_each_commit_graft(add_graft_decoration, NULL);
}
diff --git a/log-tree.h b/log-tree.h
index c8116e6..bb46c53 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,7 +25,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **subject_p,
const char **extra_headers_p,
int *need_8bit_cte_p);
-void load_ref_decorations(int flags);
+void load_ref_decorations(int flags, int reflog);
#define FORMAT_PATCH_NAME_MAX 64
void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
diff --git a/pretty.c b/pretty.c
index 5e68383..ec8e1cc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1189,11 +1189,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
strbuf_addstr(sb, get_revision_mark(NULL, commit));
return 1;
case 'd':
- load_ref_decorations(DECORATE_SHORT_REFS);
+ load_ref_decorations(DECORATE_SHORT_REFS, 0);
format_decorations(sb, commit, c->auto_color);
return 1;
case 'D':
- load_ref_decorations(DECORATE_SHORT_REFS);
+ load_ref_decorations(DECORATE_SHORT_REFS, 0);
format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
return 1;
case 'g': /* reflog info */
diff --git a/revision.c b/revision.c
index b37dbec..4d5cbf5 100644
--- a/revision.c
+++ b/revision.c
@@ -1743,7 +1743,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->simplify_by_decoration = 1;
revs->limited = 1;
revs->prune = 1;
- load_ref_decorations(DECORATE_SHORT_REFS);
+ load_ref_decorations(DECORATE_SHORT_REFS, 0);
} else if (!strcmp(arg, "--date-order")) {
revs->sort_order = REV_SORT_BY_COMMIT_DATE;
revs->topo_order = 1;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: merge maintaining history
From: David J. Bakeman @ 2017-01-19 13:12 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xoF8E55-XDnQT-GN1=hEwwq4pOsz7--P-SCy29C7ST3Hg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]
On 01/14/2017 10:24 PM, Jacob Keller wrote:
> On Fri, Jan 13, 2017 at 6:01 PM, David J. Bakeman <nakuru@comcast.net> wrote:
>> History
>>
>> git cloned a remote repository and made many changes pushing them all to
>> said repository over many months.
>>
>> The powers that be then required me to move project to new repository
>> server did so by pushing local version to new remote saving all history!
>>
>> Now have to merge back to original repository(which has undergone many
>> changes since I split off) but how do I do that without loosing the
>> history of all the commits since the original move? Note I need to push
>> changes to files that are already in existence. I found on the web a
>> bunch of ways to insert a whole new directory structure into an existing
>> repository but as I said I need to do it on top of existing files. Of
>> course I can copy all the files from my local working repository to the
>> cloned remote repository and commit any changes but I loose all the
>> history that way.
>>
>> Thanks.
> If I understand it.. you have two remotes now:
>
> The "origin" remote, which was the original remote you started with.
>
> You have now a "new" remote which you created and pushed to.
>
> So you want to merge the "new" history into the original tree now, so
> you checkout the original tree, then "git merge <new-remote>/<branch>"
> and then fix up any conflicts, and then git commit to create a merge
> commit that has the new history. Then you could push that to both
> trees.
>
> I would want a bit more information about your setup before providing
> actual commands.
Thanks I think that's close but it's a little more complicated I think
:<( I don't know if this diagram will work but lets try.
original A->B->C->D->E->F
\
first branch b->c->d->e
new repo e->f->g->h
Now I need to merge h to F without loosing b through h hopefully. Yes e
was never merged back to the original repo and it's essentially gone now
so I can't just merge to F or can I?
>
> Thanks,
> Jake
>
[-- Attachment #2: nakuru.vcf --]
[-- Type: text/x-vcard, Size: 248 bytes --]
begin:vcard
fn:David J. Bakeman
n:Bakeman;David J.
org:Nakuru Software Inc.
adr:;;1504 North 57th Street;Seattle;WA;98103;USA
email;internet:nakuru@comcast.net
tel;work:(206)545-0609
tel;fax:(206)600-6957
x-mozilla-html:TRUE
version:2.1
end:vcard
^ permalink raw reply
* Re: The design of refs backends, linked worktrees and submodules
From: Michael Haggerty @ 2017-01-19 13:30 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8CHoroX2k9GqOFmXkvvPCPN4SBeCg+6aC2WSWNSKVmWQw@mail.gmail.com>
On 01/19/2017 12:55 PM, Duy Nguyen wrote:
> I've started working on fixing the "git gc" issue with multiple
> worktrees, which brings me back to this. Just some thoughts. Comments
> are really appreciated.
>
> In the current code, files backend has special cases for both
> submodules (explicitly) and linked worktrees (hidden behind git_path).
There is another terrible hack also needed to implement linked
worktrees, namely that the `refs/bisect/` hierarchy is manually inserted
into the `ref_cache`, because otherwise it wouldn't be noticed when
iterating over loose references via `readdir()`.
Other similar hacks would be required if other reference subtrees are
declared to be per-worktree.
> But if a backend has to handle this stuff, all future backends have to
> too. Which does not sound great. Imagine we have "something" in
> addition to worktrees and submodules in future, then all backends have
> to learn about it.
Agreed, the status quo is not pretty.
I kindof think that it would have been a better design to store the
references for all linked worktrees in the main repository's ref-store.
For example, the "bisect" refs for a worktree named "<name>" could have
been stored under "refs/worktrees/<name>/bisect/*". Then either:
* teach the associated tools to read/write references there directly
(probably with DWIM rules to make command-line use easier), or
* treat these references as if they were actually at a standard place
like `refs/worktree/bisect/*`; i.e., users would need to know that they
were per-worktree references, but wouldn't need to worry about the true
locations, or
* treat these references as if they were actually in their traditional
locations (though it is not obvious how this scheme could be expanded to
cover new per-worktree references).
> So how about we move worktree and submodule support back to front-end?
>
> The backend deals with refs, period. The file-based backend will be
> given a directory where refs live in and it work on that. Backends do
> not use git_path(). Backends do not care about $GIT_DIR. Access to odb
> (e.g. sha-1 validation) if needed is abstracted out via a set of
> callbacks. This allows submodules to give access to submodule's
> separate odb. And it's getting close to the "struct repository"
> mentioned somewhere in refs "TODO" comments, even though we are
> nowhere close to introducing that struct.
This is a topic that I have thought a lot about. I definitely like this
direction. In fact I've dabbled around with some first steps; see branch
`submodule-hash` in my fork on GitHub [1]. That branch associates a
`ref_store` more closely with the directory where the references are
stored, as opposed to having a 1:1 relationship between `ref_store`s and
submodules.
I would like to see the separation of a concept "iterate over all
reachability roots" that is independent of other ref iteration. Then it
wouldn't have to include reference names, except basically for use in
error messages. So for linked worktrees, in place of the reference name
it might emit a string like "<worktree>:<refname>". (Of course it would
get its information by iterating over all of the linked reference stores
using their reference iteration APIs.)
> For that to work, I'll probably need to add a "composite" ref_store
> that combines two file-based backends (for per-repo and per-worktree
> refs) to represent a unified ref store. I think your work on ref
> iterator makes way for that. A bit worried about transactions though,
> because I think per-repo and per-worktree updates will be separated in
> two transactions. But that's probably ok because future backends, like
> lmdb, will have to go through the same route anyway.
Yes, that was the main motivation for the ref-iterator work.
Regarding transactions, the commit pointed at by branch
`split-transaction` in my fork shows how I think the
`transaction_commit()` method could be split into two parts,
`transaction_prepare()` and `transaction_finish()`. The idea would be
that the driver function, `ref_transaction_commit()`, calls
`transaction_prepare()` for each `ref_store` involved in the
transaction, passing each one the reference updates for references that
live in that reference store. Those methods would verify that the part
of the transaction that lives in that ref-store "should" go through,
without actually committing anything. Then `transaction_finish()` would
be called on each ref store, and that method would commit the updates.
You probably couldn't get a bulletproof kind of compound transaction out
of this (e.g., if the computer's power goes out, one `ref_store`'s
updates might be committed but another one's not). But it would probably
be good enough to cover everyday reasons for transaction failures, like
pre-checksums not matching up.
Let me braindump some more information about this topic. A files backend
for a repository (ignoring submodules for the moment) currently consists
of five interacting parts, each of which looks a lot like a ref-store
itself:
* A loose reference ref-store for the main repo
* A loose reference ref-store for the per-subtree references
* A ref_cache in front of the two loose reference stores
* A packed ref-store
* A second ref_cache in front of the packed ref-store
But these ref-stores are currently coupled very tightly and have
peculiarities:
* The caching code is tightly coupled to the ref-store behind it.
* It is hard to imagine a packed refs-store that doesn't have some kind
of caching in front of it.
* There are tricky ordering constraints between writes to packed and
loose references to avoid races.
* The packed ref-store can't store symbolic refs, nor can it store
reflogs. It currently relies on the loose ref-store for those things.
* There is no packed-refs ref-store associated with per-worktree refs.
* Packed references are currently locked via `*.lock` files located near
the corresponding loose references.
* There are constraints that span refstores. For example, you aren't
allowed to create a packed ref that D/F conflicts with a loose ref or
vice versa.
* Symrefs, which are loose, can point at packed references.
I've taken some stabs at picking these apart into separate ref stores,
but haven't had time to make very satisfying progress. By the time of
GitMerge I might have a better feeling for whether I can devote some
time to this project.
Michael
[1] https://github.com/mhagger/git
^ permalink raw reply
* [RFC 0/2] grep: make output consistent with revision syntax
From: Stefan Hajnoczi @ 2017-01-19 15:03 UTC (permalink / raw)
To: git; +Cc: gitster, Stefan Hajnoczi
git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
and expect "git show rev:path/to/file.c" to work. See the individual patches
for examples of command-lines that produce invalid output.
This series is an incomplete attempt at solving the issue. I'm not familiar
enough with the git codebase to propose a better solution. Perhaps someone is
interested in a proper fix?
Stefan Hajnoczi (2):
grep: only add delimiter if there isn't one already
grep: use '/' delimiter for paths
builtin/grep.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
2.9.3
^ permalink raw reply
* [RFC 2/2] grep: use '/' delimiter for paths
From: Stefan Hajnoczi @ 2017-01-19 15:03 UTC (permalink / raw)
To: git; +Cc: gitster, Stefan Hajnoczi
In-Reply-To: <20170119150347.3484-1-stefanha@redhat.com>
If the tree contains a sub-directory then git-grep(1) output contains a
colon character instead of a path separator:
$ git grep malloc v2.9.3:t
v2.9.3:t:test-lib.sh: setup_malloc_check () {
$ git show v2.9.3:t:test-lib.sh
fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
This patch attempts to use the correct delimiter:
$ git grep malloc v2.9.3:t
v2.9.3:t/test-lib.sh: setup_malloc_check () {
$ git show v2.9.3:t/test-lib.sh
(success)
This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
it as a path because it contains colons.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
builtin/grep.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 3643d8a..06f8b47 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
/* Add a delimiter if there isn't one already */
if (name[len - 1] != '/' && name[len - 1] != ':') {
- strbuf_addch(&base, ':');
+ /* rev: or rev:path/ */
+ strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
}
}
init_tree_desc(&tree, data, size);
--
2.9.3
^ permalink raw reply related
* [RFC 1/2] grep: only add delimiter if there isn't one already
From: Stefan Hajnoczi @ 2017-01-19 15:03 UTC (permalink / raw)
To: git; +Cc: gitster, Stefan Hajnoczi
In-Reply-To: <20170119150347.3484-1-stefanha@redhat.com>
git-grep(1) output does not follow git's own syntax:
$ git grep malloc v2.9.3:
v2.9.3::Makefile: COMPAT_CFLAGS += -Icompat/nedmalloc
$ git show v2.9.3::Makefile
fatal: Path ':Makefile' does not exist in 'v2.9.3'
This patch avoids emitting the unnecessary ':' delimiter if the name
already ends with ':' or '/':
$ git grep malloc v2.9.3:
v2.9.3:Makefile: COMPAT_CFLAGS += -Icompat/nedmalloc
$ git show v2.9.3:Makefile
(succeeds)
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
builtin/grep.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 462e607..3643d8a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
strbuf_init(&base, PATH_MAX + len + 1);
if (len) {
strbuf_add(&base, name, len);
- strbuf_addch(&base, ':');
+
+ /* Add a delimiter if there isn't one already */
+ if (name[len - 1] != '/' && name[len - 1] != ':') {
+ strbuf_addch(&base, ':');
+ }
}
init_tree_desc(&tree, data, size);
hit = grep_tree(opt, pathspec, &tree, &base, base.len,
--
2.9.3
^ permalink raw reply related
* Re: [RFC] stash --continue
From: Johannes Schindelin @ 2017-01-19 15:49 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Stephan Beyer, git
In-Reply-To: <38d592b8-975c-1fd9-4c42-877e34a4ab70@xiplink.com>
Hi Marc,
On Wed, 18 Jan 2017, Marc Branchaud wrote:
> On 2017-01-18 11:34 AM, Johannes Schindelin wrote:
> >
> > On Wed, 18 Jan 2017, Marc Branchaud wrote:
> >
> > > On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
> > >
> > > > On Mon, 16 Jan 2017, Stephan Beyer wrote:
> > > >
> > > > > a git-newbie-ish co-worker uses git-stash sometimes. Last time
> > > > > he used "git stash pop", he got into a merge conflict. After he
> > > > > resolved the conflict, he did not know what to do to get the
> > > > > repository into the wanted state. In his case, it was only "git
> > > > > add <resolved files>" followed by a "git reset" and a "git stash
> > > > > drop", but there may be more involved cases when your index is
> > > > > not clean before "git stash pop" and you want to have your index
> > > > > as before.
> > > > >
> > > > > This led to the idea to have something like "git stash
> > > > > --continue"[1]
> > > >
> > > > More like "git stash pop --continue". Without the "pop" command,
> > > > it does not make too much sense.
> > >
> > > Why not? git should be able to remember what stash command created
> > > the conflict. Why should I have to? Maybe the fire alarm goes off
> > > right when I run the stash command, and by the time I get back to it
> > > I can't remember which operation I did. It would be nice to be able
> > > to tell git to "just finish off (or abort) the stash operation,
> > > whatever it was".
> >
> > That reeks of a big potential for confusion.
> >
> > Imagine for example a total Git noob who calls `git stash list`,
> > scrolls two pages down, then hits `q` by mistake. How would you
> > explain to that user that `git stash --continue` does not continue
> > showing the list at the third page?
>
> Sorry, but I have trouble taking that example seriously. It assumes
> such a level of "noobness" that the user doesn't even understand how
> standard command output paging works, not just with git but with any
> shell command.
Yeah, well, I thought you understood what I meant.
The example was the best I could come up with quickly, and it only tried
to show that there are *other* stash operations that one might perceive
to happen at the same time as the "pop" operation, so your flimsical
comment "why not continue the latest operation" may very well be
ambiguous.
And if it is not ambiguous in "stash", it certainly will be in other Git
operations. And therefore, having a DWIM in "stash" to allow "--continue"
without any specific subcommand, but not having it in other Git commands,
is just a very poor user interface design. It is prone to confuse users,
which is always a hallmark of a bad user interface.
Hence my objection to "git stash --continue". No argument in favor of "git
stash --continue" I heard so far comes even close to being convincing.
> > Even worse: `git stash` (without arguments) defaults to the `save`
> > operation, so any user who does not read the documentation (and who
> > does?) would assume that `git stash --continue` *also* implies `save`.
>
> Like the first example, your user is trying to "continue" a command that
> is already complete.
Says who? You may understand the semantics better than other users, but
who are you to judge?
But that's besides the point.
My point (which you did not quite understand) was that it can be ambiguous
what to continue when looking at *all* Git commands. To special-case "git
stash"'s user interface makes things more confusing, and therefore less
usable for everyone.
And even with `git stash apply`, you could construct a very plausible
scenario (which does not work yet, but we may want to make it work): if
`git stash apply` causes conflicts, and `git stash apply stash@{1}`
conflicts in a *different* set of files, why don't we allow the second
operation to succeed (adding its conflicts)?
That example is like `git cherry-pick -n` with two different commits, both
of which conflict with the current worktree, but in different files. Both
cherry-picks would do their job if called after one another, and the
result is a worktree with the *combined* conflicts. That is a legitimate
use case (which I happened to *actually* perform just the other day).
If we fix "git stash" (and there is no reason we should not), it would
also allow "git stash pop; git stash pop" to work with two stashes that
both conflict with the current worktree, just in different files.
So I challenge you to get less hung up on the *exact* example I present,
and to try to see through the example what the issue is that I am trying
to get at.
> > If that was not enough, there would still be the overall design of
> > Git's user interface. You can call it confusing, inconsistent, with a
> > lot of room for improvement, and you would be correct. But none of
> > Git's commands has a `--continue` option that remembers the latest
> > subcommand and continues that. To introduce that behavior in `git
> > stash` would disimprove the situation.
>
> I think it's more the case that none of the current continuable commands
> have subcommands (though I can't think of all the continuable or
> abortable operations offhand, so maybe I'm wrong). I think we're
> discussing new UI ground here.
Nope, we are not entering new UI ground here. The principle is clear with
the existing --continue options: you pass them to the same operation that
you want to continue. By that reasoning, "git stash --continue" should
continue the (implicit) "save" operation. But that is not at all what you
want.
> And since the pattern is already "git foo --continue",
But foo *is the operation*! By that reasoning, you should agree that "git
stash --continue" is *wrong*!
> Think of it this way: All the currently continuable/abortable commands
> put the repository in a shaky state, where performing certain other
> operations would be ill advised. Attempting to start a rebase while a
> merge conflict is unresolved, for example. IIRC, git actually tries to
> stop users from shooting their feet in this way.
>
> And so it should be for the stash operation: If applying a stash yields
> a conflict, it has to be resolved or aborted before something like a
> rebase or merge is attempted.
That already happens, and I have no idea how you think this safe-guarding
has anything to do whether the "--continue" option makes sense in "git
stash", or only in "git stash pop".
> In the long run, I think there's even the possibility of generic "git
> continue" and "git abort" commands,
Wrong.
You can call "git cherry-pick" (and "git cherry-pick --continue") while
running a "git rebase -i".
You can run "git rebase", "git stash", "git cherry-pick" and many other
commands while running a "git bisect".
You can even run a "git rebase" or a "git cherry-pick" while resolving an
interrupted "git am".
Many, many examples that make it *impossible* for Git to know *what* you
want to continue, *what* you want to abort.
> > At least `git stash pop --continue` would be consistent with all other
> > `--continue` options in Git that I can think of...
>
> Alas, I disagree!
Sure, you are free to disagree.
And syntactially, you are even correct: "git <something> <something-else>
--continue" is inconsistent with "git <something> --continue".
But semantically, i.e. when you look at the *meaning* of those
"<something>"s, you are incorrect: the "--continue" option always goes
with the *operation* that needs to be continued. Always, always, always.
If you continue a rebase, it is "git rebase --continue", *not* "git
--continue". If you continue a revert, it is "git revert --continue". And
so it should be for popping a stash: "git stash pop --continue". Because
the operation is specified by "git stash pop", not by "git stash". There
is really not much you can argue sanely about that.
Ciao,
Johannes
^ permalink raw reply
* Re: [RFC] stash --continue
From: Johannes Schindelin @ 2017-01-19 15:54 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Marc Branchaud, git
In-Reply-To: <e273c7c8-278a-061c-44fd-4808b53d0475@gmx.net>
Hi Stephan,
On Wed, 18 Jan 2017, Stephan Beyer wrote:
> PPS: Any opinions about the mentioned "backwards-compatibility" issue
> that people are then forced to finish their commits with "--continue"
> instead of "git reset" or "git commit"?
Maybe you could make it so that "git reset" and "git commit" would still
work as before, and reset the state so that "git stash pop --continue"
could complain that there is nothing to continue?
Similar to how we remove CHERRY_PICK_HEAD during a `git reset --hard`,
maybe?
Ciao,
Johannes
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Johannes Schindelin @ 2017-01-19 16:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Jeff King, git
In-Reply-To: <xmqqfukg9o7u.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 18 Jan 2017, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
> >> So... can we move this forward?
> >
> > I have no objects anymore.
>
> Alright, thanks.
>
> Dscho what's your assessment?
I still think it will be a problem. I'll address that problem when I get
bug reports, to unblock you.
Ciao,
Johannes
^ permalink raw reply
* [PATCH] clear_delta_base_cache(): don't modify hashmap while iterating
From: Jeff King @ 2017-01-19 16:33 UTC (permalink / raw)
To: Ulrich Spörlein; +Cc: Ed Maste, git, Junio C Hamano
In-Reply-To: <CAJ9axoT1pUQc_jTKxO+RMw7emhA4ss1NCAU+hpnyG5LMwGD89w@mail.gmail.com>
On Thu, Jan 19, 2017 at 03:03:46PM +0100, Ulrich Spörlein wrote:
> > I suspect the patch below may fix things for you. It works around it by
> > walking over the lru list (either is fine, as they both contain all
> > entries, and since we're clearing everything, we don't care about the
> > order).
>
> Confirmed. With the patch applied, I can import the whole 55G in one go
> without any crashes or aborts. Thanks much!
Thanks. Here it is rolled up with a commit message.
-- >8 --
Subject: clear_delta_base_cache(): don't modify hashmap while iterating
Removing entries while iterating causes fast-import to
access an already-freed `struct packed_git`, leading to
various confusing errors.
What happens is that clear_delta_base_cache() drops the
whole contents of the cache by iterating over the hashmap,
calling release_delta_base_cache() on each entry. That
function removes the item from the hashmap. The hashmap code
may then shrink the table, but the hashmap_iter struct
retains an offset from the old table.
As a result, the next call to hashmap_iter_next() may claim
that the iteration is done, even though some items haven't
been visited.
The only caller of clear_delta_base_cache() is fast-import,
which wants to clear the cache because it is discarding the
packed_git struct for its temporary pack. So by failing to
remove all of the entries, we still have references to the
freed packed_git.
To make things even more confusing, this doesn't seem to
trigger with the test suite, because it depends on
complexities like the size of the hash table, which entries
got cleared, whether we try to access them before they're
evicted from the cache, etc.
So I've been able to identify the problem with large
imports like freebsd's svn import, or a fast-export of
linux.git. But nothing that would be reasonable to run as
part of the normal test suite.
We can fix this easily by iterating over the lru linked list
instead of the hashmap. They both contain the same entries,
and we can use the "safe" variant of the list iterator,
which exists for exactly this case.
Let's also add a warning to the hashmap API documentation to
reduce the chances of getting bit by this again.
Reported-by: Ulrich Spörlein <uqs@freebsd.org>
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-hashmap.txt | 4 +++-
sha1_file.c | 9 ++++-----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt
index 28f5a8b71..a3f020cd9 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -188,7 +188,9 @@ Returns the removed entry, or NULL if not found.
`void *hashmap_iter_next(struct hashmap_iter *iter)`::
`void *hashmap_iter_first(struct hashmap *map, struct hashmap_iter *iter)`::
- Used to iterate over all entries of a hashmap.
+ Used to iterate over all entries of a hashmap. Note that it is
+ not safe to add or remove entries to the hashmap while
+ iterating.
+
`hashmap_iter_init` initializes a `hashmap_iter` structure.
+
diff --git a/sha1_file.c b/sha1_file.c
index 1eb47f611..d20714d6b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
void clear_delta_base_cache(void)
{
- struct hashmap_iter iter;
- struct delta_base_cache_entry *entry;
- for (entry = hashmap_iter_first(&delta_base_cache, &iter);
- entry;
- entry = hashmap_iter_next(&iter)) {
+ struct list_head *lru, *tmp;
+ list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
+ struct delta_base_cache_entry *entry =
+ list_entry(lru, struct delta_base_cache_entry, lru);
release_delta_base_cache(entry);
}
}
--
2.11.0.747.g2c5918130
^ permalink raw reply related
* Re: [PATCH v5 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-19 16:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqbmv49o3s.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 18 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Junio,
> >
> > On Tue, 17 Jan 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>
> >> > It served its purpose, but now we have a builtin difftool. Time for the
> >> > Perl script to enjoy Florida.
> >> >
> >> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> > ---
> >>
> >> The endgame makes a lot of sense. Both in the cover letter and in
> >> the previous patch you talk about having both in the released
> >> version, so do you want this step to proceed slower than the other
> >> two?
> >
> > I did proceed that slowly. Already three Git for Windows versions have
> > been released with both.
> >
> > But I submitted this iteration with this patch, so my intent is clearly to
> > retire the Perl script.
>
> Ok, I was mostly reacting to 2/3 while I am reading it:
>
> The reason: this new, experimental, builtin difftool will be shipped as
> part of Git for Windows v2.11.0, to allow for easier large-scale
> testing, but of course as an opt-in feature.
>
> as there is no longer an opportunity to participate in this opt-in
> testing, unless 3/3 is special cased and delayed.
Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
obvious to you that I simply failed to spot and fix that oversight.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0
From: Jeff King @ 2017-01-19 16:38 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <20170119114123.23784-2-pclouds@gmail.com>
On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> In this code we want to match the word "reset". If len is zero,
> strncasecmp() will return zero and we incorrectly assume it's "reset" as
> a result.
This is probably a good idea. This _is_ user-visible, so it's possible
somebody was using empty config as a synonym for "reset". But since it
was never documented, I feel like relying on that is somewhat crazy.
-Peff
^ permalink raw reply
* Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()
From: Jeff King @ 2017-01-19 16:41 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <20170119114123.23784-3-pclouds@gmail.com>
On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Normally color_parse_mem() is called from config parser which trims the
> leading spaces already. The new caller in the next patch won't. Let's be
> tidy and trim leading spaces too (we already trim trailing spaces before
> comma).
What comma? I don't think that exists until the next patch. :)
I think just trimming from the front is OK, though, because
color_parse_mem() trims trailing whitespace after a word. So either you
have a word and we will trim after it, or you do not (in which case
this will trim everything and hit the !len case you added).
So maybe a better commit message is just:
Normally color_parse_mem() is called from config parser which trims
the leading spaces already. The new caller in the next patch won't.
Let's be tidy and trim leading spaces too (we already trim trailing
spaces after a word).
-Peff
^ permalink raw reply
* Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors
From: Jeff King @ 2017-01-19 16:51 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <20170119114123.23784-4-pclouds@gmail.com>
On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:
> +static void parse_graph_colors_config(struct argv_array *colors, const char *string)
> +{
> + const char *end, *start;
> +
> + start = string;
> + end = string + strlen(string);
> + while (start < end) {
> + const char *comma = strchrnul(start, ',');
> + char color[COLOR_MAXLEN];
> +
> + if (!color_parse_mem(start, comma - start, color))
> + argv_array_push(colors, color);
> + else
> + warning(_("ignore invalid color '%.*s' in log.graphColors"),
> + (int)(comma - start), start);
> + start = comma + 1;
> + }
> + argv_array_push(colors, GIT_COLOR_RESET);
> +}
This looks good.
> @@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)
> {
> struct git_graph *graph = xmalloc(sizeof(struct git_graph));
>
> - if (!column_colors)
> - graph_set_column_colors(column_colors_ansi,
> - column_colors_ansi_max);
> + if (!column_colors) {
> + struct argv_array ansi_colors = {
> + column_colors_ansi,
> + column_colors_ansi_max + 1
> + };
Hrm. At first I thought this would cause memory corrution, because your
argv_array_clear() would try to free() the non-heap array you've stuffed
inside. But you only clear the custom_colors array which actually is
dynamically allocated. This outer one is just here to give uniform
access:
> + struct argv_array *colors = &ansi_colors;
> + char *string;
> +
> + if (!git_config_get_string("log.graphcolors", &string)) {
> + static struct argv_array custom_colors = ARGV_ARRAY_INIT;
> + argv_array_clear(&custom_colors);
> + parse_graph_colors_config(&custom_colors, string);
> + free(string);
> + colors = &custom_colors;
> + }
> + /* graph_set_column_colors takes a max-index, not a count */
> + graph_set_column_colors(colors->argv, colors->argc - 1);
> + }
Since there's only one line that cares about the result of "colors",
maybe it would be less confusing to do:
if (!git_config_get-string("log.graphcolors", &string)) {
... parse, etc ...
graph_set_column_colors(colors.argv, colors.argc - 1);
} else {
graph_set_column_colors(column_colors_ansi,
column_colors_ansi_max);
}
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Johannes Schindelin @ 2017-01-19 16:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Philip Oakley, Jacob Keller, git, Johannes Sixt, Jacob Keller
In-Reply-To: <xmqqr3403x1r.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 18 Jan 2017, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
> >>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
> >>> + Introduce an option with string argument.
> >>> + The string argument is stored as an element in `&list` which must be a
> >>> + struct string_list. Reset the list using `--no-option`.
> >>> +
> >>
> >> I do not know if it is clear enough that 'option' in the last
> >> sentence is a placeholder. I then wondered if spelling it as
> >> `--no-<long>` would make it a bit clearer, but that is ugly.
> >
> > Bikeshedding:: `--no-<option>` maybe, i.e. just surround the option
> > word with the angle brackets to indicate it is to be replaced by the
> > real option's name.
>
> Yeah, I bikeshedded that myself, and rejected it because there is no
> <option> mentioned anywhere in the enumeration header.
As I pointed out in a previous review round: the surrounding test uses
--no-option (I agree that it is tedious to go back to the original code
for review, rather than a mere patch review that lacks context, but I
suspected that Jake did not come up with that `--no-option` himself), so
by our own recommendation (imitate the surrounding, existing code/text)
Jake did exactly the right thing:
$ git grep -e --no-option upstream/master -- Documentation/technical/api-parse-options.txt
upstream/master:Documentation/technical/api-parse-options.txt: `--option` and set to zero with `--no-option`.
upstream/master:Documentation/technical/api-parse-options.txt: (even if initially negative), and `--no-option` resets it to
upstream/master:Documentation/technical/api-parse-options.txt: zero. To determine if `--option` or `--no-option` was encountered at
upstream/master:Documentation/technical/api-parse-options.txt: `--no-option` was seen.
upstream/master:Documentation/technical/api-parse-options.txt: reset to zero with `--no-option`.
Ciao,
Johannes
^ permalink raw reply
* Re: [RFC 0/2] grep: make output consistent with revision syntax
From: Jeff King @ 2017-01-19 16:59 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, gitster
In-Reply-To: <20170119150347.3484-1-stefanha@redhat.com>
On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
> git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
>
> This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> and expect "git show rev:path/to/file.c" to work. See the individual patches
> for examples of command-lines that produce invalid output.
I think this is a good goal.
I couldn't immediately think of any cases where your patches would
misbehave, but my initial thought was that the "/" versus ":"
distinction is about whether the initial object is a tree or a commit.
You do still have to special case the root tree (so "v2.9.3:" does not
get any delimiter). I think "ends in a colon" is actually a reasonable
way of determining that.
> This series is an incomplete attempt at solving the issue. I'm not familiar
> enough with the git codebase to propose a better solution. Perhaps someone is
> interested in a proper fix?
Are there cases you know that aren't covered by your patches?
-Peff
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Ulrich Spörlein @ 2017-01-19 14:03 UTC (permalink / raw)
To: Jeff King; +Cc: Ed Maste, git, Junio C Hamano
In-Reply-To: <20170118215120.6hle2uxgkcvvtlox@sigill.intra.peff.net>
2017-01-18 22:51 GMT+01:00 Jeff King <peff@peff.net>:
>
> On Wed, Jan 18, 2017 at 03:27:04PM -0500, Jeff King wrote:
>
> > On Wed, Jan 18, 2017 at 09:20:07PM +0100, Ulrich Spörlein wrote:
> >
> > > I found your commit via bisect in case you were wondering. Thanks for
> > > picking this up.
> >
> > Still downloading. However, just looking at the code, the obvious
> > culprit would be clear_delta_base_cache(), which is called from
> > literally nowhere except fast-import, and then only when checkpointing.
Sorry for the bad URL, pesky last minute changes ...
> Hmm. I haven't reproduced your exact issue, but I was able to produce
> some hijinks in that function.
>
> The problem is that the hashmap_iter interface is unreliable if entries
> are added or removed from the map during the iteration.
>
> I suspect the patch below may fix things for you. It works around it by
> walking over the lru list (either is fine, as they both contain all
> entries, and since we're clearing everything, we don't care about the
> order).
Confirmed. With the patch applied, I can import the whole 55G in one go
without any crashes or aborts. Thanks much!
>
> ---
> sha1_file.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1eb47f611..d20714d6b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
>
> void clear_delta_base_cache(void)
> {
> - struct hashmap_iter iter;
> - struct delta_base_cache_entry *entry;
> - for (entry = hashmap_iter_first(&delta_base_cache, &iter);
> - entry;
> - entry = hashmap_iter_next(&iter)) {
> + struct list_head *lru, *tmp;
> + list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
> + struct delta_base_cache_entry *entry =
> + list_entry(lru, struct delta_base_cache_entry, lru);
> release_delta_base_cache(entry);
> }
> }
> --
> 2.11.0.698.gd6b48ab4c
>
>
>
>
> >
> > -Peff
^ permalink raw reply
* Re: [PATCH] log: new option decorate reflog of remote refs
From: Jeff King @ 2017-01-19 17:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20170119122630.27645-1-pclouds@gmail.com>
On Thu, Jan 19, 2017 at 07:26:30PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This is most useful when you fork your branches off a remote ref and
> rely on ref decoration to show your fork points in `git log`. Then you
> do a "git fetch" and suddenly the remote decoration is gone because
> remote refs are moved forward. With this, we can still see something
> like "origin/foo@{1}"
>
> This is for remote refs only because based on my experience, docorating
> local reflog is just too noisy. You will most likely see HEAD@{1},
> HEAD@{2} and so on. We can add that as a separate option in future if we
> see a need for it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> I've been using this for many weeks and it has proven its usefulness
> (to me). Looks like good material to send upstream.
I think it's a neat idea, but the actual option:
> +--decorate-remote-reflog[=<n>]::
> + Decorate `<n>` most recent reflog entries on remote refs, up
> + to the specified number of entries. By default, only the most
> + recent reflog entry is decorated.
seems weirdly limited and non-orthogonal. What happens when somebody
wants to decorate other reflogs besides refs/remotes?
We already have very flexible ref-selectors like --remotes, --branches,
etc. The generalization of this would perhaps be something like:
git log --decorate-reflog --remotes --branches
where "--decorate-reflog" applies to the next ref selector and then is
reset, the same way --exclude is. And it includes those refs _only_ for
decoration, not for traversal. So you could do:
git log --decorate-reflog --remotes --remotes
if you wanted to see use those as traversal roots, too (if this is
common, it might even merit another option for "decorate and show").
That's just off the top of my head, so maybe there are issues. I was
just surprised to see the "-remote" part in your option name.
-Peff
^ permalink raw reply
* Re: [PATCH v5 3/3] Retire the scripted difftool
From: Junio C Hamano @ 2017-01-19 17:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <alpine.DEB.2.20.1701191730040.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Ok, I was mostly reacting to 2/3 while I am reading it:
>>
>> The reason: this new, experimental, builtin difftool will be shipped as
>> part of Git for Windows v2.11.0, to allow for easier large-scale
>> testing, but of course as an opt-in feature.
>>
>> as there is no longer an opportunity to participate in this opt-in
>> testing, unless 3/3 is special cased and delayed.
>
> Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
> obvious to you that I simply failed to spot and fix that oversight.
OK, if you want to tweak log message of either 2/3 or 3/3 to
correct, there is still time, as they are still outside 'next'.
I am hoping we can merge it and others to 'next' by the end of the
week, though.
^ permalink raw reply
* grep open pull requests
From: Jack Bates @ 2017-01-19 18:12 UTC (permalink / raw)
To: git
I have a couple questions around grepping among open pull requests.
First, "git for-each-ref --no-merged": When I run the following,
it lists refs/pull/1112/head, even though #1112 was merged in commit
ced4da1. I guess this is because the tip of refs/pull/1112/head is
107fc59, not ced4da1?
This maybe shows my lack of familiarity with Git details,
but superficially the two commits appear identical -- [1] and [2] --
same parent, etc. Nonetheless they have different SHA-1s.
I'm not sure why that is -- I didn't merge the commit --
but regardless, GitHub somehow still connects ced4da1 with #1112.
So my question is, how are they doing that,
and why can't "git for-each-ref --no-merged" likewise
connect ced4da1 with refs/pull/1112/head?
$ git clone \
> --bare \
> --config remote.origin.fetch=+refs/pull/*/head:refs/pull/*/head \
> https://github.com/apache/trafficserver.git
$ cd trafficserver.git
$ git for-each-ref --no-merged
More generally, what I want is to periodically list open pull requests
that add or modify lines containing the string "memset". So far I have
the following in a Makefile. Can you recommend any improvements?
.PHONY: all
all: trafficserver.git
cd trafficserver.git && git fetch
cd trafficserver.git && git for-each-ref --format '%(refname)'
refs/pull --no-merged | \
while read refname; do \
git log -p "master..$$refname" | grep -q '^+.*memset' && echo
"$$refname"; \
done
trafficserver.git:
git clone \
--bare \
--config remote.origin.fetch=+refs/pull/*/head:refs/pull/*/head \
https://github.com/apache/trafficserver.git
Lastly, a question more about GitHub than Git, but: Given the way GitHub
is setup, I hope I can get a list of unmerged pull requests from Git
alone. Can you think of a way to list *open* pull requests,
or is that status only available out of band?
Thanks!
[1]
https://github.com/apache/trafficserver/commit/107fc59104cce2a4b527f04e7ac86695c98b568c
[2]
https://github.com/apache/trafficserver/commit/ced4da13279f834c381925f2ecd1649bfb459e8b
^ permalink raw reply
* Re: Git: new feature suggestion
From: Junio C Hamano @ 2017-01-19 18:17 UTC (permalink / raw)
To: Konstantin Khomoutov
Cc: Joao Pinto, git, Linus Torvalds, CARLOS.PALMINHA@synopsys.com
In-Reply-To: <20170119093313.ea57832dfd1bc7e0b0f1e630@domain007.com>
Konstantin Khomoutov <kostix+git@007spb.ru> writes:
> Still, I welcome you to read the sort-of "reference" post by Linus
> Torvalds [1] in which he explains the reasoning behind this approach
> implemented in Git. IMO, understanding the reasoning behind the idea
> is much better than just mechanically learning how to use it.
>
> The whole thread (esp. Torvalds' replies) is worth reading, but that
> particular mail summarizes the whole thing very well.
>
> (The reference link to it used to be [2], but Gmane is not fully
> recovered to be able to display it.)
>
> 1. http://public-inbox.org/git/Pine.LNX.4.58.0504150753440.7211@ppc970.osdl.org/
> 2. http://thread.gmane.org/gmane.comp.version-control.git/27/focus=217
Indeed. Thanks for providing a link to it here ;-)
The message is the most important one in the early history of Git,
and it still is one of the most important messages in the Git
mailing-list archive. "git log -S<block>" was designed to take a
block of text (even though people misuse it and feed a single line
to it) exactly because it wanted to serve the "tracking when that
file+line changed" part in that vision. The rename detection in
"diff" was meant to be used on the commit "git log -S<block>" finds
to see if the found change came from another file so that the user
can decide that "digging further" part needs to be done for another
file. "git blame" with -M and -C options were done to mostly
automate the "drilling down" process that finds the last commit that
touched each line in the above process, and when used with tools
like "tig", you can even peel one commit back and "zoom down" if the
found commit is an uninteresting one (e.g. a change with only code
formatting).
One thing that is still missing in the current version of Git,
compared to the "ideal SCM" the message envisioned, is the part that
notices: "oops, that line didn't even exist in the previous version,
BUT I FOUND FIVE PLACES that matched almost perfectly in the same
diff, and here they are".
^ permalink raw reply
* Re: [RFC 0/2] grep: make output consistent with revision syntax
From: Brandon Williams @ 2017-01-19 18:26 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Hajnoczi, git, gitster
In-Reply-To: <20170119165958.xtotlvdta7udqllb@sigill.intra.peff.net>
On 01/19, Jeff King wrote:
> On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
>
> > git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> >
> > This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> > and expect "git show rev:path/to/file.c" to work. See the individual patches
> > for examples of command-lines that produce invalid output.
>
> I think this is a good goal.
I agree.
> I couldn't immediately think of any cases where your patches would
> misbehave, but my initial thought was that the "/" versus ":"
> distinction is about whether the initial object is a tree or a commit.
I think this is also the case, I couldn't think of another case where
this decision wasn't based on if the object is a tree or a commit.
Interestingly enough I don't think we have any tests that exist that
test the formatting of grep's output when given a tree object since the
test suite still passes with these changes in. Which means this fix
should probably include a couple tests to ensure there's no regression
in the future.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()
From: Junio C Hamano @ 2017-01-19 18:22 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170119164157.mvoadhxxwwynedoz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> Normally color_parse_mem() is called from config parser which trims the
>> leading spaces already. The new caller in the next patch won't. Let's be
>> tidy and trim leading spaces too (we already trim trailing spaces before
>> comma).
>
> What comma? I don't think that exists until the next patch. :)
>
> I think just trimming from the front is OK, though, because
> color_parse_mem() trims trailing whitespace after a word. So either you
> have a word and we will trim after it, or you do not (in which case
> this will trim everything and hit the !len case you added).
>
> So maybe a better commit message is just:
>
> Normally color_parse_mem() is called from config parser which trims
> the leading spaces already. The new caller in the next patch won't.
> Let's be tidy and trim leading spaces too (we already trim trailing
> spaces after a word).
>
> -Peff
Yeah, my reading stuttered at the same place in the original, and
your rewrite looks a lot more sensible.
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Jeff King @ 2017-01-19 18:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <alpine.DEB.2.20.1701181700020.3469@virtualbox>
On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:
> > > I want to err on the side of caution. That's why.
> >
> > I guess I just don't see why changing the behavior with respect to
> > "prune" or "proxy" is any less conservative than changing the one for
> > "refspec".
>
> Let's take a step back and consider the problem I try to solve, okay?
OK. Though after reading your message over several times, I think I may
have confused things by raising two separate issues.
So let me re-state my issues here for clarity:
1. I'm concerned that a setting for remote.<url>.X would fail to apply
to a bare "git fetch <url>" if "X" is considered as "not really"
configuring a remote.
2. I'm concerned that splitting the remote.*.X keys into two classes:
"really configures" and "does not really configure" creates a
confusing interface for users. Some keys are OK to set in your
~/.gitconfig and others are not.
Let's talk about concern 1 first. Based on your analysis in this
message, it looks like it _isn't_ a problem with your patch (because
is_configured is never used for applying values, only for add/del/rename
types of operations in remote.c).
So good. Thanks for addressing my concern. And that makes your
"conservative" comment make more sense; the idea is that you are not
breaking anything, but just loosening selectively for some values of
"X".
I think there are still some weird corner cases even for "prune",
though. E.g., try:
git init
git remote add foo whatever
git config remote.foo.prune true
git config remote.other.prune false
So now we have:
[remote "foo"]
url = whatever
fetch = +refs/heads/*:refs/remotes/foo/*
prune = true
[remote "other"]
prune = false
Now try "git remote rename foo other". With current versions of git,
it's disallowed. With your patch, I get:
[remote "other"]
url = whatever
prune = true
[remote "other"]
prune = false
fetch = +refs/heads/*:refs/remotes/other/*
The old value of remote.other.prune is overriding the one we tried to
move into place.
In your motivating example, the old value is in ~/.gitconfig, so it
automatically takes lower precedence, and everything just works (I think
it would also just work if "other" had been defined originally _before_
foo in the config file).
I think you could fix it by having git-remote teach the "not really"
config values (like "prune") to overwrite any existing value when
rewriting the config. I think this just needs the multi_replace flag set
when calling git_config_set_multivar().
That raises questions about what should happen when multi-value keys
like "refspec" would be set (if we were to add them to the "not really"
set). Should they be overwritten, or merged? And in that sense, your
patch lets you punt on those issues.
I still think my second concern is valid. It's made worse by your patch
(if only because everything was disallowed before, and now some things
are and some things aren't). If this is a first step towards a final
state where the rules are simple, then starting conservatively makes
sense. And until we get there, the answer "yes, it should, but nobody
has worked out the semantics yet; care to make a patch?" is an OK one.
But it sounds like you do not ever want to loosen the "refspec" case.
I don't think that's ideal, but at the very least if that's the end
state, the list of "OK to use in ~/.gitconfig" keys should probably be
documented.
Reading your message, though, I still wonder if we can do better...
> The problem is that `git remote rename <old> <new>` refuses to do its job
> if it detects a configured `<new>`. And it detects a configured `<new>`
> even in cases where there is not *really* any remote configured.
I'd add to this that "git remote add <new>" should work in a similar way
(that was the one that I think people often ran into with
remote.origin.fetch refspecs).
> The example use case is to configure `remote.origin.prune = true` in
> ~/.gitconfig, i.e. changing Git's default for all "origin" remotes in all
> of the user's repositories.
>
> Now, the *real* fix would be to detect whether the remote was "configured"
> in the current repository, or in ~/.gitconfig. But that may not even be
> desirable, as we would want a more general fix for the question: can `git
> remote` rename a given remote to a new name, i.e. is that new name already
> taken?
I think _this_ is a much better way of framing the problem. It is not
about which keys are set, but about _where_ they are set. IOW, a
reasonable rule would be: if there is any remote.*.X in the repo config,
then git-remote should consider it a configured repo. And otherwise, no
matter what is in ~/.gitconfig or elsewhere, git-remote should proceed
as if it doesn't exist (and repo-level config can take precedence over
config defined elsewhere).
I.e., something like this:
diff --git a/remote.c b/remote.c
index 298f2f93f..720d616be 100644
--- a/remote.c
+++ b/remote.c
@@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)
}
remote = make_remote(name, namelen);
remote->origin = REMOTE_CONFIG;
+ if (current_config_scope() == CONFIG_SCOPE_REPO)
+ remote->configured = 1;
if (!strcmp(subkey, "mirror"))
remote->mirror = git_config_bool(key, value);
else if (!strcmp(subkey, "skipdefaultupdate"))
That doesn't make your test pass, but I think that is only because your
test is not covering the interesting case (it puts the new config in the
repo config, not in ~/.gitconfig).
What do you think?
> Originally, I would even have put the "vcs" into that set, as I could see
> a legitimate use case for users to configure "remote.svn.vcs = vcs" in
> ~/.gitconfig. But the regression test suite specifically tests for that
> case, and I trust that there was a good reason, even if Thomas did not
> describe that good reason in the commit message nor in any reply to this
> patch pair.
The config-scope thing above would allow "remote.svn.vcs" in
~/.gitconfig. But I don't think the test script actually checks that; it
checks for the repo-level config. And we would continue to do the right
thing there.
-Peff
^ 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