* [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
* Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors
From: Junio C Hamano @ 2017-01-19 18:27 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170119165127.6cxw64fjz7aevkq2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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);
> }
Yes, that would be much much less confusing. By doing so, the cover
letter can lose "pushing the limits of abuse", too ;-).
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Junio C Hamano @ 2017-01-19 17:58 UTC (permalink / raw)
To: Jacob Keller
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <CA+P7+xrv35w0RYNEZ88K_EWC57A4utBQTibtw6+TJBCtzA9Ybw@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> 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.
>
> To be fair, this is exactly how the rest of the doc spells these
> things, so I would rather be consistent with the doc as is, and a
> future patch could clean this up. See OPT_SET_INT, for an example of
> `--no-option`.
Ah, OK, then I have no issues with it.
> Maybe we can rephrase this "The list is reset via `--no-option`"?
I think I saw that in your latest interdiff and I think it is good.
Thanks.
^ permalink raw reply
* Re: [RFC] stash --continue
From: Marc Branchaud @ 2017-01-19 18:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, git
In-Reply-To: <alpine.DEB.2.20.1701191341530.3469@virtualbox>
On 2017-01-19 10:49 AM, Johannes Schindelin wrote:
> 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.
Please don't underestimate the power of syntactic consistency in helping
users achieve their goals. Having some commands use "git foo
--continue" while others use "git foo bar --continue" *will* confuse
people, regardless of how logical the reasons for those differences.
But in the case of stash, I still don't see the utility in having
operation-specific continuation. Consider the following sequence (as
you say, this doesn't work yet, but making it work seems reasonable):
git stash pop # creates some conflicts
git stash apply stash@{4} # creates some other conflicts
# (User resolves the conflicts created by the pop.)
git stash pop --continue
Given the description of the original proposal (do "git reset; git stash
drop"), what's the state of the index and the working tree?
In particular, what has the user gained by continuing just that pop?
Another thing to ask is, how common is such a scenario likely to be? I
suggest that it will be far more common for users to resolve all the
conflicts and then want to continue all their interrupted stash
operations. If so, fussily forcing them to explicitly continue the pop
and the apply is just a waste.
> 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.
Well, what about the potential for a slippery slope? If the user is
forced to be specific about continuing either a pop or an apply, why
wouldn't git allow them to be specific about *which* pop or apply they
want to continue? Consider another hypothetical scenario:
git stash pop # creates some conflicts
git stash pop # creates some more conflicts
git stash pop # creates even more conflicts
# (User resolves the conflicts created by second pop.)
git stash pop --continue
# Oops, there's still some unresovled pops!
Obviously the user isn't ready to finish off all the pops, so they'll
want some way to specify which pop to continue. Dealing with that just
feels like a lot of work for minimal benefit.
>>> 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)?
Running those two commands should be perfectly fine. The interesting
question is what it means to *continue* from that state.
> 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).
I don't mean to suggest that you shouldn't be able to do that.
> 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.
Sounds fine to me. So, what would it mean to continue from that state?
> 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*!
No, in the user's mind *stash* is the operation! The user is doing
"stash" stuff. She doesn't care if the conflicts came from a pop or an
apply. She has resolved the conflicts, and now she just wants to continue.
There's no confusion about possibly continuing some other stash
subcommand, like save or list or drop. None of those other commands are
continuable. In the following sequence
git stash "let me save my work now"
git stash --continue
That continue command does nothing, regardless of whether the implied
save command succeeded or failed. There's simply nothing to continue.
With the sequence
git stash pop # creates some conflicts
git stash "got some popped conflicts"
git stash --continue
Again the continue command should do nothing: If the save command
succeeded it should have cleaned up the interrupted pop. If the save
command failed, then the continue command can't continue from a
conflicted state. (OTOH, a "git stash --abort" would abort the pop.)
>> 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.
Right, I'd missed that. I agree that a generic "git continue" is nonsense.
M.
^ permalink raw reply
* Re: Git: new feature suggestion
From: Joao Pinto @ 2017-01-19 17:55 UTC (permalink / raw)
To: Konstantin Khomoutov, Joao Pinto
Cc: git, Linus Torvalds, CARLOS.PALMINHA@synopsys.com
In-Reply-To: <20170119093313.ea57832dfd1bc7e0b0f1e630@domain007.com>
Hi,
Às 6:33 AM de 1/19/2017, Konstantin Khomoutov escreveu:
> On Wed, 18 Jan 2017 10:40:52 +0000
> Joao Pinto <Joao.Pinto@synopsys.com> wrote:
>
> [...]
>> I have seen a lot of Linux developers avoid this re-organization
>> operations because they would lose the renamed file history, because
>> a new log is created for the new file, even if it is a renamed
>> version of itself. I am sending you this e-mail to suggest the
>> creation of a new feature in Git: when renamed, a file or folder
>> should inherit his parent’s log and a “rename: …” would be
>> automatically created or have some kind of pointer to its “old” form
>> to make history analysis easier.
>
> Git does not record renames because of its stance that what matters is
> code _of the whole project_ as opposed to its location in a particular
> file.
>
> Hence with regard to renames Git "works backwards" by detecting them
> dynamically while traversing the history (such as with `git log`
> etc). This detection uses certain heuristics which can be controlled
> with knobs pointed to by Stefan Beller.
>
> 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. https://urldefense.proofpoint.com/v2/url?u=http-3A__public-2Dinbox.org_git_Pine.LNX.4.58.0504150753440.7211-40ppc970.osdl.org_&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=X0bQCOGTuZF-uq6smPwJDw4Q47qHgjWaewgTHCbhMnM&s=97U97toe9A6XOAJxbhxvWeYpzl-wPw9QvlhQfAEUTdI&e=
> 2. https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.version-2Dcontrol.git_27_focus-3D217&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=X0bQCOGTuZF-uq6smPwJDw4Q47qHgjWaewgTHCbhMnM&s=agYFOBCbLeaKAB6frWWzcwHkZyrMZLW4ExgDxzQyVlI&e=
>
Thank you very much for the info!
Joao
^ permalink raw reply
* Re: Git: new feature suggestion
From: Linus Torvalds @ 2017-01-19 18:39 UTC (permalink / raw)
To: Konstantin Khomoutov
Cc: Joao Pinto, Git Mailing List, CARLOS.PALMINHA@synopsys.com
In-Reply-To: <20170119093313.ea57832dfd1bc7e0b0f1e630@domain007.com>
On Wed, Jan 18, 2017 at 10:33 PM, Konstantin Khomoutov
<kostix+git@007spb.ru> wrote:
>
> 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.
It's worth noting that that discussion was from some _very_ early days
in git (one week into the whole thing), when none of those
visualization tools were actually implemented.
Even now, ten years after the fact, plain git doesn't actually do what
I outlined. Yes, "git blame -Cw" works fairly well, and is in general
better than the traditional per-file "annotate". And yes, "git log
--follow" does another (small) part of the outlined thing, but is
really not very powerful.
Some tools on top of git do more, but I think in general this is an
area that could easily be improved upon. For example, the whole
iterative and interactive drilling down in history of a particular
file is very inconvenient to do with "git blame" (you find a commit
that change the area in some way that you don't find interesting, so
then you have to restart git blame with the parent of that
unintersting commit).
You can do it in tig, but I suspect a more graphical tool might be better.
.. and we still end up having a lot of things where we simply just
work with pathnames. For example, when doing merges, it' absolutely
_wonderful_ doing
gitk --merge <filename>
to see what happened to that filename that has a conflict during the
merge. But it's all based on the whole-file changes, and sometimes
you'd like to see just the commits that generate one particular
conflict (in the kernel, things like the MAINTAINERS file can have
quite a lot of changes, but they are all pretty idnependent, and what
you want to see is just "changes to this area").
We do have the "-L" flag to git log, but it doesn't actually work for
this particular case because of limitations.
So what I'm trying to say is that the argument from 10+ years ago that
"you can do better with intelligent tools after-the-fact" is very much
true, but it's also true that we don't actually have all those
intelligent tools, and this is an area that could still be improved
upon. Some of them are actually available as add-ons in various
graphical IDE's that use git.
Linus
^ permalink raw reply
* Re: [RFC 1/2] grep: only add delimiter if there isn't one already
From: Junio C Hamano @ 2017-01-19 18:39 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git
In-Reply-To: <20170119150347.3484-2-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> 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)
I am mildly negative on this one. I suspect that the above example
is a made-up one and you might have a more compelling real-world use
case in mind, but at least the above does not convince me.
The end-user explicitly gave the extra ':' because she wanted to
feed the tree object, not a tag that leads to the tree, for some
reason. I think the output should respect that and show how she
spelled the starting point. IOW, it is not "':' added unnecessarily
by Git". It is ':' added by the end-user for whatever reason.
>
> 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,
^ 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