* [PATCH] restore: invalidate cache-tree when removing entries with --staged
From: Jeff King @ 2020-01-08 11:43 UTC (permalink / raw)
To: Torsten Krah; +Cc: Junio C Hamano, Emily Shaffer, git@vger.kernel.org
In-Reply-To: <20200108104008.GA2207365@coredump.intra.peff.net>
On Wed, Jan 08, 2020 at 05:40:08AM -0500, Jeff King wrote:
> So there seem to be at least two bugs:
>
> - git-restore doesn't properly invalidate the cache-tree
>
> - the index-reading code is not careful enough about bogus cache-trees,
> and may segfault
Here's a fix for the first one. I'm adding Junio to the cc as an expert
in index and cache-tree issues. I'm pretty sure this is the correct fix,
but I have some lingering questions below.
I'm not planning on working on the second one immediately. Between this
and Emily's patch from yesterday, I have a feeling that the index code
could use an audit to be a bit more careful about handling bogus on-disk
data.
-- >8 --
Subject: restore: invalidate cache-tree when removing entries with --staged
When "git restore --staged <path>" removes a path that's in the index,
it marks the entry with CE_REMOVE, but we don't do anything to
invalidate the cache-tree. In the non-staged case, we end up in
checkout_worktree(), which calls remove_marked_cache_entries(). That
actually drops the entries from the index, as well as invalidating the
cache-tree and untracked-cache.
But with --staged, we never call checkout_worktree(), and the CE_REMOVE
entries remain. Interestingly, they are dropped when we write out the
index, but that means the resulting index is inconsistent: its
cache-tree will not match the actual entries, and running "git commit"
immediately after will create the wrong tree.
We can solve this by calling remove_marked_cache_entries() ourselves
before writing out the index. Note that we can't just hoist it out of
checkout_worktree(); that function needs to iterate over the CE_REMOVE
entries (to drop their matching worktree files) before removing them.
One curiosity about the test: without this patch, it actually triggers a
BUG() when running git-restore:
BUG: cache-tree.c:810: new1 with flags 0x4420000 should not be in cache-tree
But in the original problem report, which used a similar recipe,
git-restore actually creates the bogus index (and the commit is created
with the wrong tree). I'm not sure why the test here behaves differently
than my out-of-suite reproduction, but what's here should catch either
symptom (and the fix corrects both cases).
Reported-by: Torsten Krah <krah.tm@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
So I'm mildly puzzled about the BUG thing above. But also: it seems
really weird that do_write_index() will drop CE_REMOVE entries as it
writes, but not invalidate the cache-tree (or the untracked-cache, which
AFAICT is probably similarly affected). Should it be doing that
invalidation? Or should it be a BUG() to write out an index without
having done remove_marked_cache_entries()?
builtin/checkout.c | 2 ++
t/t2070-restore.sh | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b52c490c8f..18ef5fb975 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -524,6 +524,8 @@ static int checkout_paths(const struct checkout_opts *opts,
/* Now we are committed to check them out */
if (opts->checkout_worktree)
errs |= checkout_worktree(opts);
+ else
+ remove_marked_cache_entries(&the_index, 1);
/*
* Allow updating the index when checking out from the index.
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 21c3f84459..06a5976143 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -96,6 +96,7 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
'
test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '
+ test_when_finished git reset --hard &&
echo "nonempty" >nonempty &&
>empty &&
git add nonempty empty &&
@@ -106,4 +107,21 @@ test_expect_success 'restore --staged adds deleted intent-to-add file back to in
git diff --cached --exit-code
'
+test_expect_success 'restore --staged invalidates cache tree for deletions' '
+ test_when_finished git reset --hard &&
+ >new1 &&
+ >new2 &&
+ git add new1 new2 &&
+
+ # It is important to commit and then reset here, so that the index
+ # contains a valid cache-tree for the "both" tree.
+ git commit -m both &&
+ git reset --soft HEAD^ &&
+
+ git restore --staged new1 &&
+ git commit -m "just new2" &&
+ git rev-parse HEAD:new2 &&
+ test_must_fail git rev-parse HEAD:new1
+'
+
test_done
--
2.25.0.rc1.622.g8cfac75bdd
^ permalink raw reply related
* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Johannes Schindelin @ 2020-01-08 10:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <xmqqtv573twq.fsf@gitster-ct.c.googlers.com>
Hi Junio,
I will change GitGitGadget to no longer Cc: you automatically.
Please register my suspicion that this will make GitGitGadget a lot less
useful: the stated mission of GitGitGadget is to make contributing patches
to the Git project _easier_ so that the contributor can focus on the
changes they want to make, rather than on the rather involved process.
I know, you do not find any fault with the current process; it works for
you. It just does not work all that well for many other people, myself
included. The sheer amount of mostly unwritten, and not exactly static
rules contributors are expected to follow are starting to remind me of
Kafka's "The Trial".
On Tue, 7 Jan 2020, Junio C Hamano wrote:
> Besides, when they send out patches they would also add area experts and
> those who participated in the review of the earlier round to Cc: so GGG
> needs to have a mechanism to allow the end user to do so.
So GitGitGadget should now also learn to determine who the current area
experts are???
I must have misread your request.
> And by treating the maintainer merely just one of the reviewer, that
> mechanism can naturally be reused.
Well, I certainly do not treat you as just one of the reviewers, as your
complaints definitely keep me on my tip toes with regards to GitGitGadget.
I do have to remind myself frequently that only two people ever complained
about GitGitGadget, literally everybody else who is using GitGitGadget is
quite happy. So maybe I should listen more to those positive voices.
Actually, now that I wrote it, I think that is the only sane course of
action here: listen more to positive voices.
Ciao,
Dscho
^ permalink raw reply
* Re: Broken branch after git commit - tracked files in staging area can't be removed with restore --staged, or commit or stash
From: Jeff King @ 2020-01-08 10:40 UTC (permalink / raw)
To: Torsten Krah; +Cc: Emily Shaffer, git@vger.kernel.org
In-Reply-To: <2423f8c0b91578c0faf7527b7d97b0e1e9666261.camel@gmail.com>
On Wed, Jan 08, 2020 at 11:02:41AM +0100, Torsten Krah wrote:
> Hi Jeff, I have a poc you can try:
Thanks, I can see the problem now.
> cd /tmp
> mkdir testrepo
> cd testrepo
> touch TEST1 TEST2
> git add -A
> git commit -m First
> touch TEST3 TEST4
> git add -A
> git commit -m Second
> git reset --soft HEAD~1
OK, so we'd expect to still see TEST3 and TEST4 in the index. And we do:
> git status
>
> Auf Branch master
> Zum Commit vorgemerkte Änderungen:
> (benutzen Sie "git restore --staged <Datei>..." zum Entfernen aus
> der Staging-Area)
> neue Datei: TEST3
> neue Datei: TEST4
And then if we try to unstage one of them, we should see that:
> git restore --staged TEST3
>
> [10:57:26][tkrah@torstenknbl:/tmp/testrepo] (master) $ LC_ALL=C git
> status
> On branch master
> Changes to be committed:
> (use "git restore --staged <file>..." to unstage)
> new file: TEST4
>
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
> TEST3
So far so good. But I think there is a lurking problem in the index,
because...
> git commit -m Second
>
> [master 5b62331] Second
> 2 files changed, 0 insertions(+), 0
> deletions(-)
> create mode 100644 TEST3
> create mode 100644 TEST4
This is very wrong (and is a bug, not a problem with what you're doing).
We wrote a commit with TEST3 in it, even though it wasn't in the index.
Why would we do that? Almost certainly it's because of a cache-tree
extension in the index. Presumably "git restore --staged" is not
properly invalidating the cache-tree entry. And that would explain what
you see next:
> And now TEST3 is in the commit and what is even more "interesting" is
> the next one:
>
> [10:59:16][tkrah@torstenknbl:/tmp/testrepo] (master) $ LC_ALL=C git
> status
> On branch master
> Changes to be committed:
> (use "git restore --staged <file>..." to unstage)
> deleted: TEST3
>
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
> TEST3
>
> TEST3 is unstaged and deleted now.
That happens because we wrote out a commit with TEST3, but it's still
not in the index! So it appears as if a deletion was staged (remember
that what is "staged" is really just the diff between the index and
HEAD).
We can repeat the same process, substituting "git reset -- TEST3" for
git-restore, and it works fine. So the problem is in git-restore itself.
Here's another observation. If we do:
git restore --staged TEST3 TEST4
then running "git commit" won't do anything, since there's nothing
staged (and this is why I had trouble reproducing the problem earlier).
If we add a new file, like:
echo whatever >new
git add new
then the problem won't exhibit, because that will also invalidate the
cache-tree (because it has to account for "new"). But we could put those
files in their own subdirectory and do:
$ git restore --staged subdir/TEST3
$ git status
On branch master
Changes to be committed:
new file: subdir/TEST4
Untracked files:
subdir/TEST3
and if I commit that, I see the bug again:
$ git commit -m second
[master 5c3b8c1] second
2 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 subdir/TEST3
create mode 100644 subdir/TEST4
Now here's even more fun. I wanted to do another test, so I tried to
reset back to the original "second" commit, but it segfaults! Yikes.
$ git reset --hard HEAD@{2}
Segmentation fault
Running it under valgrind shows we're indeed seeing a bogus cache-tree:
==2214443== Invalid read of size 4
==2214443== at 0x362DFF: traverse_by_cache_tree (unpack-trees.c:734)
==2214443== by 0x3630A3: traverse_trees_recursive (unpack-trees.c:799)
==2214443== by 0x364209: unpack_callback (unpack-trees.c:1258)
==2214443== by 0x35F925: traverse_trees (tree-walk.c:497)
==2214443== by 0x364DDD: unpack_trees (unpack-trees.c:1589)
==2214443== by 0x1BE6CF: reset_index (reset.c:95)
==2214443== by 0x1BF894: cmd_reset (reset.c:421)
==2214443== by 0x124868: run_builtin (git.c:444)
==2214443== by 0x124BAE: handle_builtin (git.c:674)
==2214443== by 0x124E24: run_argv (git.c:741)
==2214443== by 0x1252AB: cmd_main (git.c:872)
==2214443== by 0x1E811A: main (common-main.c:52)
==2214443== Address 0x40 is not stack'd, malloc'd or (recently) free'd
==2214443==
==2214443== Process terminating with default action of signal 11 (SIGSEGV)
This isn't the same spot that Emily fixed earlier today in [1], but it
seems like a very similar problem. Weird.
So instead I blew away the index and then did hard reset:
$ rm .git/index
$ git reset --hard
HEAD is now at 5c3b8c1 second
And now we can see what happens if both files are unstaged, and then we
add a new file, and then commit:
$ git reset --soft HEAD^
$ git restore --staged subdir/TEST3 subdir/TEST4
$ echo whatever >new
$ git add new
$ git commit -m again
[master 119d9e4] again
1 file changed, 1 insertion(+)
create mode 100644 new
So that _doesn't_ exhibit the problem. I guess because adding the new
file causes git-add to reexamine the whole cache-tree and clean it up.
So there seem to be at least two bugs:
- git-restore doesn't properly invalidate the cache-tree
- the index-reading code is not careful enough about bogus cache-trees,
and may segfault
-Peff
[1] https://lore.kernel.org/git/20200108023127.219429-1-emilyshaffer@google.com/
^ permalink raw reply
* Re: Broken branch after git commit - tracked files in staging area can't be removed with restore --staged, or commit or stash
From: Torsten Krah @ 2020-01-08 10:31 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <2423f8c0b91578c0faf7527b7d97b0e1e9666261.camel@gmail.com>
Am Mittwoch, den 08.01.2020, 11:02 +0100 schrieb Torsten Krah:
> cd testrepo
git init
of cause after this one ;)
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Christian Couder @ 2020-01-08 10:28 UTC (permalink / raw)
To: Miriam R.; +Cc: brian m. carlson, Jeff King, git, Junio C Hamano
In-Reply-To: <CAN7CjDCv2uYNaHEtQv6Zco33-Cba1Fn2by87NU_3vwytZHy_WA@mail.gmail.com>
On Wed, Jan 8, 2020 at 10:16 AM Miriam R. <mirucam@gmail.com> wrote:
>
> El mié., 8 ene. 2020 a las 3:47, brian m. carlson
> (<sandals@crustytoothpaste.net>) escribió:
> > > I think Miriam actually posted the same patch in her initial email:
> > >
> > > https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/
> > >
> > > I don't know how we want to handle authorship.
> >
> > I don't feel strongly about holding authorship; I'm happy to have her
> > name on it instead of mine since she did propose a solution. I just
> > care that we fix it.
> my mentor (Christian Couder <chriscool@tuxfamily.org>) was who
> detected the problem and sent me the solution, I only asked the
> community. I think his name should be in the patch instead of mine.
I am ok with not having my name anywhere and leaving the patch as is.
^ permalink raw reply
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
From: Eric Sunshine @ 2020-01-08 10:22 UTC (permalink / raw)
To: Emily Shaffer
Cc: Junio C Hamano, Heba Waly via GitGitGadget, Git List, Heba Waly
In-Reply-To: <20200108014442.GC181522@google.com>
On Tue, Jan 7, 2020 at 8:44 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > (It is rather verbose and ugly, though.)
> >
> > I tend to agree. It also feels to me that it is giving too much
> > hand-holding, but after all advise() may turning out to be about
> > giving that.
>
> Well, if advise() isn't going to hold their hand, who is? ;)
> What I mean is, I think that's indeed what advise() is about, and the
> reason it can be disabled in config.
Git is already drowning in configuration options. Every new option
increases the complexity level for the user and of Git overall,
requires additional code, additional tests, and additional
documentation, and must be maintained for the life of the project. So,
every configuration option carries a cost in terms of end-user
experience and developer resources.
If anything, we should be trying to keep the number of configuration
options constant (or, even better, reduce it), rather than forever
adding more. Thus, we should think very hard before adding any new
configuration option, trying instead to discover ways in which an
issue can be addressed without adding a configuration option (with its
attendant costs and complexity), and only add an option as a last
resort.
> To me, the harm of giving too much hand-holding seems less than the
> harm of giving not enough; to deal with the one requires skimming
> past things you already know [...]
Perhaps I'm too old-school and too steeped in The Unix Way, but there
is value in silence and conciseness, and that value outweighs
chattiness, especially when chattiness is effectively unnecessary
(which is the case in the vast majority of error/warning messages
emitted by Git).
Too much hand-holding quickly becomes noise which gets ignored, thus
eventually drowns out the really important information. People need to
understand a problem before taking action to solve it. Blindly
following some piece of advice without understanding a problem can
lead to further problems (especially in those cases when it's not
possible to devise advice which suitably covers all situations in
which a particular problem may arise).
A well-written, clear, _concise_, error message can often provide
enough information to allow the user to understand and resolve the
problem without additional hand-holding. It's true that there are some
complex situations (for instance, detached HEAD) for which additional
hints can be handy for newcomers or casual users, but that should be
the exception, not the norm. Rather than adding advice messages for
every possible problem, perhaps a better use of our time would be to
weed out poorly-worded and unhelpful error messages and improve them.
^ permalink raw reply
* Re: Broken branch after git commit - tracked files in staging area can't be removed with restore --staged, or commit or stash
From: Torsten Krah @ 2020-01-08 10:02 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <20200108091119.GB87523@coredump.intra.peff.net>
Am Mittwoch, den 08.01.2020, 04:11 -0500 schrieb Jeff King:
> That step seems wrong, and I can't reproduce it here. If "git status"
> lists the files as unstaged, then "git commit" should not be
> committing
> them. Can you show us a more complete example that we can run
> ourselves
> (i.e., that does not rely on whatever is in "main", and what is in
> $FILES)? Barring that, can you show us the output of the commands, as
> well as "git show FETCH_HEAD FETCH_HEAD~1"?
>
> -Peff
Hi Jeff, I have a poc you can try:
cd /tmp
mkdir testrepo
cd testrepo
touch TEST1 TEST2
git add -A
git commit -m First
touch TEST3 TEST4
git add -A
git commit -m Second
git reset --soft HEAD~1
git status
Auf Branch master
Zum Commit vorgemerkte Änderungen:
(benutzen Sie "git restore --staged <Datei>..." zum Entfernen aus
der Staging-Area)
neue Datei: TEST3
neue Datei: TEST4
git restore --staged TEST3
[10:57:26][tkrah@torstenknbl:/tmp/testrepo] (master) $ LC_ALL=C git
status
On branch master
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
new file: TEST4
Untracked files:
(use "git add <file>..." to include in what will be committed)
TEST3
git commit -m Second
[master 5b62331] Second
2 files changed, 0 insertions(+), 0
deletions(-)
create mode 100644 TEST3
create mode 100644 TEST4
And now TEST3 is in the commit and what is even more "interesting" is
the next one:
[10:59:16][tkrah@torstenknbl:/tmp/testrepo] (master) $ LC_ALL=C git
status
On branch master
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
deleted: TEST3
Untracked files:
(use "git add <file>..." to include in what will be committed)
TEST3
TEST3 is unstaged and deleted now.
This seems wrong - or did I something wrong?
Cheers
Torsten
--
Mit freundlichen Grüßen / Best regards
Torsten Krah
mgm technology partners GmbH
Neumarkt 2
04109 Leipzig
Tel. +49 (341) 339 893-539
E-Mail Torsten.Krah@mgm-tp.com
Innovation Implemented.
Geschäftsführer / CEO: Hamarz Mehmanesh
Sitz der Gesellschaft / Registered office: München
Handelsregister/ Commercial register: AG München HRB 161298
USt-IdNr. / VAT ID: DE815309575
^ permalink raw reply
* Re: Fwd: Add support for axiterm colors in parse_color
From: Jeff King @ 2020-01-08 9:52 UTC (permalink / raw)
To: Eyal Soha; +Cc: git
In-Reply-To: <CANsz78LEZiocv_E-Hvj3iBahFFgomPb3BFNdmas2iqxqRLfRiw@mail.gmail.com>
On Tue, Jan 07, 2020 at 10:36:53AM -0500, Eyal Soha wrote:
> https://en.wikipedia.org/wiki/ANSI_escape_code
>
> ANSI color codes 90-97 and 100-107 are brighter versions of the 30-37
> and 40-47 colors. To view them, run `colortest-16`. In that
> colortest, they are named with a leading "+". Maybe git config could
> support those colors, too, with a leading plus in the name? They are
> nice for having a different shade of a color in a diff.
Depending on your terminal, you can already access these colors via
256-color mode. Generally 0-7 are the standard colors there, then 8-15
are the "bright" variants, and then an rgb cube.
You can see how your terminal displays all of them with something like
this:
reset=$(git config --type=color --default=reset foo.bar)
for i in $(seq 0 255); do
color=$(git config --type=color --default="$i" foo.bar)
echo "$color$i$reset"
done
Some terminals also show "bold red" as bright red, but many modern ones
seem to actually provide a bold font.
That said, I'm not entirely opposed to having more human-readable
aliases. I'm not sure if it's worth using the 16-color version (e.g.,
"ESC[91m" versus the 256-color version "ESC[38;5;9m"). It's possible it
would be compatible with more terminals, and it is slightly fewer bytes.
> diff --git a/color.c b/color.c
> index ebb222ec33..a39fe7d133 100644
> --- a/color.c
> +++ b/color.c
> @@ -33,6 +33,7 @@ struct color {
> COLOR_UNSPECIFIED = 0,
> COLOR_NORMAL,
> COLOR_ANSI, /* basic 0-7 ANSI colors */
> + COLOR_AXITERM, /* brighter than 0-7 ANSI colors */
What's AXITERM? I couldn't find it mentioned anywhere around ANSI codes.
I kind of wonder if we could just call these ANSI as well, and have
color_output() just decide whether to add an offset of 60 when we see a
color number between 8 and 15. Or possibly c->value should just have the
60-offset value in the first place.
> * already have the ANSI escape code in it. "out" should have enough
> * space in it to fit any color.
> */
> -static char *color_output(char *out, int len, const struct color *c, char type)
> +static char *color_output(char *out, int len, const struct color *c,
> + const char *type)
This "type" field was already pretty ugly, and I think your patch makes
it even worse. It would probably be less bad if we just passed in the
integer 30 instead of '3', and then do:
/* handles ANSI; your bright colors would want to add 60 */
out += xsnprintf(out, len, "%d", type + c->value);
And then likewise the 256-color and rgb paths would do:
out += xsnprintf(out, len, "%d;5;%d", type + 8, c->value);
Bonus points for declaring:
enum {
COLOR_FOREGROUND = 30,
COLOR_BACKGROUND = 40,
} color_ground;
to make the caller more readable.
> case COLOR_ANSI:
> - if (len < 2)
> + case COLOR_AXITERM:
> + if (len < strlen(type) + 1)
> BUG("color parsing ran out of space");
> - *out++ = type;
> - *out++ = '0' + c->value;
> + out += xsnprintf(out, len, "%s%c", type, '0' + c->value);
This BUG() can go away. The point was to make sure we don't overflow
"out", but xsnprintf() will check that for us (that's why there isn't
one in the COLOR_256 and COLOR_RGB case arms).
> @@ -279,14 +287,24 @@ int color_parse_mem(const char *value, int
> value_len, char *dst)
> if (!color_empty(&fg)) {
> if (sep++)
> OUT(';');
> - /* foreground colors are all in the 3x range */
> - dst = color_output(dst, end - dst, &fg, '3');
> + /* foreground colors are in the 3x range */
> + char *range = "3";
> + if (fg.type == COLOR_AXITERM) {
> + /* axiterm fg colors are in the 9x range */
> + range = "9";
> + }
> + dst = color_output(dst, end - dst, &fg, range);
And if we can handle the regular/bright stuff instead of color_output(),
we don't need to have this extra fg.type check.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
From: Eric Sunshine @ 2020-01-08 9:27 UTC (permalink / raw)
To: Heba Waly; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano
In-Reply-To: <CACg5j26jyWnAtM+mZ-FuN7OQWHpKk5nADG+7J-=metJMdO6+2Q@mail.gmail.com>
On Tue, Jan 7, 2020 at 8:15 PM Heba Waly <heba.waly@gmail.com> wrote:
> On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > By the way, did you actually run across a real-world case in which
> > someone was confused about how to resolve this situation? I ask
> > because this almost seems like too much hand-holding, and it would be
> > nice to avoid polluting Git with unnecessary advice.
>
> No I didn't. I was trying to find scenarios where git can give more
> user-friendly messages to its users.
> I see your point though, so I don't mind not proceeding with this
> patch if the community doesn't think it's adding any value.
My own feeling is that this level of hand-holding is unnecessary, at
least until we a discover a good number of real-world cases in which
people are baffled by how to deal with this situation. Adding the
advice seems simple on the surface, but every new piece of advice
means having to add yet another configuration variable, writing more
code, more tests, and more documentation, and it needs to be
maintained for the life of the project. So what seems simple at first
glance, can end up being costly in terms of developer resources. For a
bit of advice which doesn't seem to be needed by anyone (yet), all
that effort seem unwarranted. Thus, my preference is to see the patch
dropped.
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Miriam R. @ 2020-01-08 9:15 UTC (permalink / raw)
To: brian m. carlson, Jeff King, git, Junio C Hamano, Miriam R.
In-Reply-To: <20200108024732.GL6570@camp.crustytoothpaste.net>
El mié., 8 ene. 2020 a las 3:47, brian m. carlson
(<sandals@crustytoothpaste.net>) escribió:
>
> On 2020-01-07 at 11:01:45, Jeff King wrote:
> > > Noticed-by: Miriam R. <mirucam@gmail.com>
> > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> >
> > I think Miriam actually posted the same patch in her initial email:
> >
> > https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/
> >
> > I don't know how we want to handle authorship.
>
> I don't feel strongly about holding authorship; I'm happy to have her
> name on it instead of mine since she did propose a solution. I just
> care that we fix it.
> --
Hi,
my mentor (Christian Couder <chriscool@tuxfamily.org>) was who
detected the problem and sent me the solution, I only asked the
community. I think his name should be in the patch instead of mine.
Best,
Miriam
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204
^ permalink raw reply
* Re: Broken branch after git commit - tracked files in staging area can't be removed with restore --staged, or commit or stash
From: Jeff King @ 2020-01-08 9:11 UTC (permalink / raw)
To: Torsten Krah; +Cc: git
In-Reply-To: <f0638fc0d09c213b661d2b244d3457f362daebe0.camel@gmail.com>
On Tue, Jan 07, 2020 at 04:28:24PM +0100, Torsten Krah wrote:
> I can reproduce that (locally) at least:
>
> What does *not* work for me:
>
> git clone XX main
> cd main
> git fetch XX && git checkout FETCH_HEAD
> git checkout -b TEST
> git reset --soft HEAD~1
> git restore --staged $FILES
>
> git status now lists $FILES as unstaged and they are not included in
> the staging area.
>
> git commit
>
> -> now $FILES are included in the commit (I would expect them not to be
> included - right?) and git status does list those still in the working
> area.
That step seems wrong, and I can't reproduce it here. If "git status"
lists the files as unstaged, then "git commit" should not be committing
them. Can you show us a more complete example that we can run ourselves
(i.e., that does not rely on whatever is in "main", and what is in
$FILES)? Barring that, can you show us the output of the commands, as
well as "git show FETCH_HEAD FETCH_HEAD~1"?
-Peff
^ permalink raw reply
* Re: [PATCH 1/1] Preserve topological ordering after merges This modifies the algorithm of topopological ordering. The problem with the current algoritm is that it displays the graph below as follows
From: Sergey Rudyshin @ 2020-01-08 7:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Sergey Rudyshin via GitGitGadget, git, Junio C Hamano
In-Reply-To: <nycvar.QRO.7.76.6.2001071305330.46@tvgsbejvaqbjf.bet>
Hi, Johannes,
thank you for your comment
Indeed i forgot to put an emty line. Will fix it.
On Tue, Jan 7, 2020 at 3:08 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Sergey,
>
> please note that the commit message's first line should not be longer than
> 76 characters per line, and it should be followed by an empty line. I
> think that you forgot the empty line, looking at
> https://github.com/gitgitgadget/git/pull/514/commits/542a02020c8578d0e004cb9c929bced8719b902a
>
> Ciao,
> Johannes
>
> On Tue, 7 Jan 2020, Sergey Rudyshin via GitGitGadget wrote:
>
> > From: Sergey Rudyshin <540555@gmail.com>
> >
> > * E
> > |\
> > | * D
> > | |\
> > | |/
> > |/|
> > * | C
> > | * B
> > |/
> > * A
> >
> > commit B is placed between A and C, which is wrong
> > because E stays that D and B comes after C
> >
> > the only correct solution here is
> >
> > * E
> > |\
> > | * D
> > | |\
> > | |/
> > |/|
> > | * B
> > * | C
> > |/
> > * A
> >
> > while it seems that it contradicts to
> > D stating that C should be between D and B
> > The new algorithm solves this issue
> >
> > Here is an example:
> > * walk to to the root via "first" parents;
> > * go E -> C -> A;
> > * print A because it has no parents;
> > * step back to C;
> > * print C because it has no other parents;
> > * then step back to E;
> > * go D -> B -> A;
> > * do not print A because A is already printed;
> > * step back to B;
> > * print B;
> > * so on.
> >
> > This change makes option "--topo-order" obsolete, because
> > there is only one way to order parents of a single commit.
> > "--date-order" and "--author-date-order" are preserved and make sense
> > only for the case when multiple commits are given
> > to be able to sort those commits.
> >
> > Signed-off-by: Sergey Rudyshin <540555@gmail.com>
> > ---
> > Documentation/git-rev-list.txt | 4 +-
> > Documentation/rev-list-options.txt | 41 +++---
> > commit.c | 149 ++++++++++++++-------
> > commit.h | 4 +-
> > t/t4202-log.sh | 15 +--
> > t/t4215-log-skewed-merges.sh | 44 +++---
> > t/t6003-rev-list-topo-order.sh | 54 ++++----
> > t/t6012-rev-list-simplify.sh | 8 +-
> > t/t6016-rev-list-graph-simplify-history.sh | 41 +++---
> > t/t6600-test-reach.sh | 6 +-
> > 10 files changed, 214 insertions(+), 152 deletions(-)
> >
> > diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
> > index 025c911436..ad1fda7a54 100644
> > --- a/Documentation/git-rev-list.txt
> > +++ b/Documentation/git-rev-list.txt
> > @@ -3,7 +3,7 @@ git-rev-list(1)
> >
> > NAME
> > ----
> > -git-rev-list - Lists commit objects in reverse chronological order
> > +git-rev-list - Lists commit objects in reverse topological order
> >
> >
> > SYNOPSIS
> > @@ -17,7 +17,7 @@ DESCRIPTION
> > List commits that are reachable by following the `parent` links from the
> > given commit(s), but exclude commits that are reachable from the one(s)
> > given with a '{caret}' in front of them. The output is given in reverse
> > -chronological order by default.
> > +topological order by default.
> >
> > You can think of this as a set operation. Commits given on the command
> > line form a set of commits that are reachable from any of them, and then
> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> > index bfd02ade99..7ab2f451ae 100644
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -643,39 +643,42 @@ ifndef::git-shortlog[]
> > Commit Ordering
> > ~~~~~~~~~~~~~~~
> >
> > -By default, the commits are shown in reverse chronological order.
> > +By default, the commits are shown in reverse topological order.
> > ++
> > +Meaning that no parents are shown before all of its children and
> > +merges do not change the previous order.
> > ++
> > +E.g., if commit M0 produces topologically ordered list of
> > +commits L0 then commit M1 created by merging any commit into M0
> > +produces list L1 which starts with L0
> > ++
> > +In case if multiple commits are given via the command line
> > +the following parameters determine the order among them:
> >
> > --date-order::
> > - Show no parents before all of its children are shown, but
> > - otherwise show commits in the commit timestamp order.
> > + show commits in the commit timestamp order.
> >
> > --author-date-order::
> > - Show no parents before all of its children are shown, but
> > - otherwise show commits in the author timestamp order.
> > + show commits in the author timestamp order.
> >
> > --topo-order::
> > - Show no parents before all of its children are shown, and
> > - avoid showing commits on multiple lines of history
> > - intermixed.
> > + show commits in the commit timestamp order.
> > + this is deprecated and will be
> > + removed in the future.
> > +
> > For example, in a commit history like this:
> > +
> > ----------------------------------------------------------------
> > -
> > - ---1----2----4----7
> > - \ \
> > - 3----5----6----8---
> > -
> > + 1----2-----------5
> > + \ \ /
> > + -----\---3---4---6
> > + \ /
> > + ----
> > ----------------------------------------------------------------
> > +
> > where the numbers denote the order of commit timestamps, `git
> > rev-list` and friends with `--date-order` show the commits in the
> > -timestamp order: 8 7 6 5 4 3 2 1.
> > -+
> > -With `--topo-order`, they would show 8 6 5 3 7 4 2 1 (or 8 7 4 2 6 5
> > -3 1); some older commits are shown before newer ones in order to
> > -avoid showing the commits from two parallel development track mixed
> > -together.
> > +following order: 1 2 3 4 5 6.
> >
> > --reverse::
> > Output the commits chosen to be shown (see Commit Limiting
> > diff --git a/commit.c b/commit.c
> > index 434ec030d6..01e9d07db9 100644
> > --- a/commit.c
> > +++ b/commit.c
> > @@ -738,6 +738,12 @@ int compare_commits_by_author_date(const void *a_, const void *b_,
> > return 0;
> > }
> >
> > +static int compare_commits_by_author_date_rev(const void *a_, const void *b_,
> > + void *cb_data)
> > +{
> > + return compare_commits_by_author_date(b_, a_, cb_data);
> > +}
> > +
> > int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused)
> > {
> > const struct commit *a = a_, *b = b_;
> > @@ -767,36 +773,83 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)
> > return 0;
> > }
> >
> > +static int compare_commits_by_commit_date_rev(const void *a_, const void *b_, void *unused)
> > +{
> > + return compare_commits_by_commit_date (b_, a_, unused);
> > +}
> > +
> > +struct maze {
> > + struct commit *ret;
> > + struct commit *out;
> > +};
> > +
> > +define_commit_slab(maze_slab, struct maze *);
> > +
> > +/*
> > + * returns the next commit while exploring the maze
> > + * current commit has the information:
> > + * - what was the last commit we went for (OUT)
> > + * - from which commit we came in (RET)
> > + * trying to find a parent next to OUT
> > + * if it does not exists returns RET
> > + * otherwise returns the found parent
> > + */
> > +static struct commit *get_next(struct commit *commit, struct maze *mz, struct indegree_slab *indegree)
> > +{
> > + struct commit_list *parents = commit->parents;
> > + struct commit *next = NULL;
> > + int found = 0;
> > +
> > + while (parents) {
> > + struct commit *parent = parents->item;
> > +
> > + if (*indegree_slab_at(indegree, parent)) {
> > +
> > + if (!mz->out || found) {
> > + next = parent;
> > + break;
> > + } else if (mz->out == parent) {
> > + found = 1;
> > + }
> > + }
> > + parents = parents->next;
> > + }
> > +
> > + if (!next) next = mz->ret;
> > +
> > + return next;
> > +}
> > +
> > /*
> > * Performs an in-place topological sort on the list supplied.
> > */
> > void sort_in_topological_order(struct commit_list **list, enum rev_sort_order sort_order)
> > {
> > struct commit_list *next, *orig = *list;
> > - struct commit_list **pptr;
> > + struct commit_list **pptr = list;
> > struct indegree_slab indegree;
> > - struct prio_queue queue;
> > + struct prio_queue queue_tips;
> > struct commit *commit;
> > struct author_date_slab author_date;
> > + struct maze_slab maze;
> > + struct maze *mz;
> >
> > if (!orig)
> > return;
> > *list = NULL;
> >
> > init_indegree_slab(&indegree);
> > - memset(&queue, '\0', sizeof(queue));
> > + init_maze_slab(&maze);
> > + memset(&queue_tips, '\0', sizeof(queue_tips));
> >
> > switch (sort_order) {
> > - default: /* REV_SORT_IN_GRAPH_ORDER */
> > - queue.compare = NULL;
> > - break;
> > - case REV_SORT_BY_COMMIT_DATE:
> > - queue.compare = compare_commits_by_commit_date;
> > + default:
> > + queue_tips.compare = compare_commits_by_commit_date_rev;
> > break;
> > case REV_SORT_BY_AUTHOR_DATE:
> > init_author_date_slab(&author_date);
> > - queue.compare = compare_commits_by_author_date;
> > - queue.cb_data = &author_date;
> > + queue_tips.compare = compare_commits_by_author_date_rev;
> > + queue_tips.cb_data = &author_date;
> > break;
> > }
> >
> > @@ -804,9 +857,10 @@ void sort_in_topological_order(struct commit_list **list, enum rev_sort_order so
> > for (next = orig; next; next = next->next) {
> > struct commit *commit = next->item;
> > *(indegree_slab_at(&indegree, commit)) = 1;
> > - /* also record the author dates, if needed */
> > - if (sort_order == REV_SORT_BY_AUTHOR_DATE)
> > - record_author_date(&author_date, commit);
> > + mz = xmalloc(sizeof(struct maze));
> > + mz->ret = NULL;
> > + mz->out = NULL;
> > + *(maze_slab_at(&maze, commit)) = mz;
> > }
> >
> > /* update the indegree */
> > @@ -832,51 +886,56 @@ void sort_in_topological_order(struct commit_list **list, enum rev_sort_order so
> > for (next = orig; next; next = next->next) {
> > struct commit *commit = next->item;
> >
> > - if (*(indegree_slab_at(&indegree, commit)) == 1)
> > - prio_queue_put(&queue, commit);
> > + if (*(indegree_slab_at(&indegree, commit)) == 1) {
> > + /* also record the author dates, if needed */
> > + if (sort_order == REV_SORT_BY_AUTHOR_DATE)
> > + record_author_date(&author_date, commit);
> > + prio_queue_put(&queue_tips, commit);
> > + }
> > }
> >
> > - /*
> > - * This is unfortunate; the initial tips need to be shown
> > - * in the order given from the revision traversal machinery.
> > - */
> > - if (sort_order == REV_SORT_IN_GRAPH_ORDER)
> > - prio_queue_reverse(&queue);
> > -
> > /* We no longer need the commit list */
> > free_commit_list(orig);
> >
> > pptr = list;
> > *list = NULL;
> > - while ((commit = prio_queue_get(&queue)) != NULL) {
> > - struct commit_list *parents;
> > -
> > - for (parents = commit->parents; parents ; parents = parents->next) {
> > - struct commit *parent = parents->item;
> > - int *pi = indegree_slab_at(&indegree, parent);
> >
> > - if (!*pi)
> > + while ((commit = prio_queue_get(&queue_tips)) != NULL) {
> > + struct commit *prev_commit = NULL;
> > + while (commit) {
> > + mz = *(maze_slab_at(&maze, commit));
> > + if (!prev_commit) {
> > + /* either a new tip or
> > + * after entering an already visited commit
> > + */
> > + }
> > + else if (!mz->out) {
> > + /* happens only once for every commit
> > + * note that for tips RET is set to NULL
> > + */
> > + mz->ret = prev_commit;
> > + }
> > + else if (prev_commit != mz->out) {
> > + /* entered an already visited commit
> > + * step back
> > + */
> > + commit = prev_commit;
> > + prev_commit = NULL;
> > continue;
> > -
> > - /*
> > - * parents are only enqueued for emission
> > - * when all their children have been emitted thereby
> > - * guaranteeing topological order.
> > - */
> > - if (--(*pi) == 1)
> > - prio_queue_put(&queue, parent);
> > + }
> > + mz->out = get_next(commit, mz, &indegree);
> > + if (mz->out == mz->ret) {
> > + /* upon leaving a commit emit it */
> > + *pptr = commit_list_insert(commit, pptr);
> > + }
> > + prev_commit = commit;
> > + commit = mz->out;
> > }
> > - /*
> > - * all children of commit have already been
> > - * emitted. we can emit it now.
> > - */
> > - *(indegree_slab_at(&indegree, commit)) = 0;
> > -
> > - pptr = &commit_list_insert(commit, pptr)->next;
> > }
> >
> > clear_indegree_slab(&indegree);
> > - clear_prio_queue(&queue);
> > + clear_maze_slab(&maze);
> > + clear_prio_queue(&queue_tips);
> > if (sort_order == REV_SORT_BY_AUTHOR_DATE)
> > clear_author_date_slab(&author_date);
> > }
> > diff --git a/commit.h b/commit.h
> > index 221cdaa34b..1fe3cc240e 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -209,7 +209,7 @@ struct commit *pop_commit(struct commit_list **stack);
> > void clear_commit_marks(struct commit *commit, unsigned int mark);
> > void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark);
> >
> > -
> > +/* TODO get rid of rev_sort_order in a favour of positional parameters */
> > enum rev_sort_order {
> > REV_SORT_IN_GRAPH_ORDER = 0,
> > REV_SORT_BY_COMMIT_DATE,
> > @@ -222,8 +222,6 @@ enum rev_sort_order {
> > * invariant of resulting list is:
> > * a reachable from b => ord(b) < ord(a)
> > * sort_order further specifies:
> > - * REV_SORT_IN_GRAPH_ORDER: try to show a commit on a single-parent
> > - * chain together.
> > * REV_SORT_BY_COMMIT_DATE: show eligible commits in committer-date order.
> > */
> > void sort_in_topological_order(struct commit_list **, enum rev_sort_order);
> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> > index 2c9489484a..46f04b36a3 100755
> > --- a/t/t4202-log.sh
> > +++ b/t/t4202-log.sh
> > @@ -635,17 +635,16 @@ test_expect_success 'set up more tangled history' '
> > cat > expect <<\EOF
> > * Merge tag 'reach'
> > |\
> > -| \
> > +| * reach
> > +| |
> > | \
> > *-. \ Merge tags 'octopus-a' and 'octopus-b'
> > |\ \ \
> > -* | | | seventh
> > | | * | octopus-b
> > -| |/ /
> > -|/| |
> > -| * | octopus-a
> > -|/ /
> > -| * reach
> > +| | |/
> > +| * / octopus-a
> > +| |/
> > +* / seventh
> > |/
> > * Merge branch 'tangle'
> > |\
> > @@ -673,7 +672,7 @@ cat > expect <<\EOF
> > * initial
> > EOF
> >
> > -test_expect_success 'log --graph with merge' '
> > +test_expect_success 'log --graph with merge tag reach' '
> > git log --graph --date-order --pretty=tformat:%s |
> > sed "s/ *\$//" >actual &&
> > test_cmp expect actual
> > diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> > index 18709a723e..91630c0cae 100755
> > --- a/t/t4215-log-skewed-merges.sh
> > +++ b/t/t4215-log-skewed-merges.sh
> > @@ -5,7 +5,7 @@ test_description='git log --graph of skewed merges'
> > . ./test-lib.sh
> >
> > check_graph () {
> > - cat >expect &&
> > + sed "s/ *$//" >expect &&
> > git log --graph --pretty=tformat:%s "$@" >actual.raw &&
> > sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > @@ -185,20 +185,21 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
> >
> > check_graph --date-order <<-\EOF
> > * 4_H
> > - |\
> > + |\
> > | * 4_G
> > - | |\
> > + | |\
> > + | | * 4_C
> > | * | 4_F
> > - |/| |
> > + |/| |
> > | * | 4_E
> > - | |\ \
> > - | | * | 4_D
> > - | |/ /
> > - |/| |
> > - | | * 4_C
> > - | |/
> > + | |\ \
> > + | | |/
> > + | |/|
> > + | | * 4_D
> > + | |/
> > + |/|
> > | * 4_B
> > - |/
> > + |/
> > * 4_A
> > EOF
> > '
> > @@ -221,21 +222,20 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
> >
> > check_graph <<-\EOF
> > * 5_H
> > - |\
> > + |\
> > | *-. 5_G
> > - | |\ \
> > + | |\ \
> > | | | * 5_F
> > | | * | 5_E
> > - | |/|\ \
> > - | |_|/ /
> > - |/| | /
> > - | | |/
> > - * | | 5_D
> > + | |/|\ \
> > + | |_|/ /
> > + |/| | /
> > + | | |/
> > | | * 5_C
> > - | |/
> > - |/|
> > - | * 5_B
> > - |/
> > + | * | 5_B
> > + | |/
> > + * / 5_D
> > + |/
> > * 5_A
> > EOF
> > '
> > diff --git a/t/t6003-rev-list-topo-order.sh b/t/t6003-rev-list-topo-order.sh
> > index 24d1836f41..19cb79bc29 100755
> > --- a/t/t6003-rev-list-topo-order.sh
> > +++ b/t/t6003-rev-list-topo-order.sh
> > @@ -87,12 +87,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > l1
> > @@ -105,15 +105,15 @@ l5
> > l4
> > l3
> > a4
> > -b4
> > -a3
> > -a2
> > c3
> > c2
> > +c1
> > +b4
> > b3
> > b2
> > -c1
> > b1
> > +a3
> > +a2
> > a1
> > a0
> > l2
> > @@ -127,12 +127,12 @@ l5
> > l4
> > l3
> > a4
> > -b4
> > c3
> > c2
> > +c1
> > +b4
> > b3
> > b2
> > -c1
> > b1
> > a3
> > a2
> > @@ -205,12 +205,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > EOF
> > @@ -224,12 +224,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > EOF
> > @@ -237,10 +237,10 @@ EOF
> > test_output_expect_success 'prune near topo' 'git rev-list --topo-order a4 ^c3' <<EOF
> > a4
> > b4
> > +b3
> > a3
> > a2
> > a1
> > -b3
> > EOF
> >
> > test_output_expect_success "head has no parent" 'git rev-list --topo-order root' <<EOF
> > @@ -297,8 +297,8 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > +b3
> > +b2
> > EOF
> >
> > test_output_expect_success "max-count 10 - non topo order" 'git rev-list --max-count=10 l5' <<EOF
> > @@ -376,12 +376,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > l1
> > @@ -399,12 +399,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > l1
> > @@ -428,12 +428,12 @@ c3
> > c2
> > c1
> > b4
> > -a3
> > -a2
> > -a1
> > b3
> > b2
> > b1
> > +a3
> > +a2
> > +a1
> > a0
> > l2
> > l1
> > diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
> > index a10f0df02b..9f687b6b22 100755
> > --- a/t/t6012-rev-list-simplify.sh
> > +++ b/t/t6012-rev-list-simplify.sh
> > @@ -122,16 +122,16 @@ check_result () {
> >
> > check_result 'L K J I H F E D C G B A' --full-history --topo-order
> > check_result 'L K I H G F E D C B J A' --full-history
> > -check_result 'L K I H G F E D C B J A' --full-history --date-order
> > -check_result 'L K I H G F E D B C J A' --full-history --author-date-order
> > +check_result 'L K J I H F E D C G B A' --full-history --date-order
> > +check_result 'L K J I H F E D C G B A' --full-history --author-date-order
> > check_result 'K I H E C B A' --full-history -- file
> > check_result 'K I H E C B A' --full-history --topo-order -- file
> > check_result 'K I H E C B A' --full-history --date-order -- file
> > -check_result 'K I H E B C A' --full-history --author-date-order -- file
> > +check_result 'K I H E C B A' --full-history --author-date-order -- file
> > check_result 'I E C B A' --simplify-merges -- file
> > check_result 'I E C B A' --simplify-merges --topo-order -- file
> > check_result 'I E C B A' --simplify-merges --date-order -- file
> > -check_result 'I E B C A' --simplify-merges --author-date-order -- file
> > +check_result 'I E C B A' --simplify-merges --author-date-order -- file
> > check_result 'I B A' -- file
> > check_result 'I B A' --topo-order -- file
> > check_result 'I B A' --date-order -- file
> > diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
> > index f5e6e92f5b..5750c6f0fd 100755
> > --- a/t/t6016-rev-list-graph-simplify-history.sh
> > +++ b/t/t6016-rev-list-graph-simplify-history.sh
> > @@ -235,27 +235,28 @@ test_expect_success '--graph ^C3' '
> > # I don't think the ordering of the boundary commits is really
> > # that important, but this test depends on it. If the ordering ever changes
> > # in the code, we'll need to update this test.
> > -test_expect_success '--graph --boundary ^C3' '
> > +test_expect_success '--graph --boundary --all ^C3' '
> > rm -f expected &&
> > - echo "* $A7" >> expected &&
> > - echo "* $A6" >> expected &&
> > - echo "|\\ " >> expected &&
> > - echo "| * $C4" >> expected &&
> > - echo "* | $A5" >> expected &&
> > - echo "| | " >> expected &&
> > - echo "| \\ " >> expected &&
> > - echo "*-. \\ $A4" >> expected &&
> > - echo "|\\ \\ \\ " >> expected &&
> > - echo "| * | | $B2" >> expected &&
> > - echo "| * | | $B1" >> expected &&
> > - echo "* | | | $A3" >> expected &&
> > - echo "o | | | $A2" >> expected &&
> > - echo "|/ / / " >> expected &&
> > - echo "o / / $A1" >> expected &&
> > - echo " / / " >> expected &&
> > - echo "| o $C3" >> expected &&
> > - echo "|/ " >> expected &&
> > - echo "o $C2" >> expected &&
> > + cat >> expected <<-TESTDATA &&
> > + * $A7
> > + * $A6
> > + |\
> > + | * $C4
> > + * | $A5
> > + | |
> > + | \
> > + *-. \ $A4
> > + |\ \ \
> > + | * | | $B2
> > + | * | | $B1
> > + * | | | $A3
> > + | | | o $C3
> > + | | |/
> > + | | o $C2
> > + o | $A2
> > + |/
> > + o $A1
> > + TESTDATA
> > git rev-list --graph --boundary --all ^C3 > actual &&
> > test_cmp expected actual
> > '
> > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> > index b24d850036..a6d389c4fd 100755
> > --- a/t/t6600-test-reach.sh
> > +++ b/t/t6600-test-reach.sh
> > @@ -339,6 +339,8 @@ test_expect_success 'rev-list: ancestry-path topo-order' '
> > run_three_modes git rev-list --topo-order --ancestry-path commit-3-3..commit-6-6
> > '
> >
> > +# TODO get rid of the "sort" below
> > +# if commitGraph is enabled then sort_in_topological_order is not envoked
> > test_expect_success 'rev-list: symmetric difference topo-order' '
> > git rev-parse \
> > commit-6-6 commit-5-6 commit-4-6 \
> > @@ -349,8 +351,8 @@ test_expect_success 'rev-list: symmetric difference topo-order' '
> > commit-6-1 commit-5-1 commit-4-1 \
> > commit-3-8 commit-2-8 commit-1-8 \
> > commit-3-7 commit-2-7 commit-1-7 \
> > - >expect &&
> > - run_three_modes git rev-list --topo-order commit-3-8...commit-6-6
> > + | sort >expect &&
> > + run_three_modes git rev-list --topo-order commit-3-8...commit-6-6 | sort
> > '
> >
> > test_expect_success 'get_reachable_subset:all' '
> > --
> > gitgitgadget
> >
> >
^ permalink raw reply
* Re: SHA-1 chosen-prefix colission attack
From: Jeff King @ 2020-01-08 7:30 UTC (permalink / raw)
To: Santiago Torres Arias; +Cc: Kevin Daudt, git
In-Reply-To: <20200107203147.r33c5plp5g7pmxmj@LykOS.localdomain>
On Tue, Jan 07, 2020 at 03:31:48PM -0500, Santiago Torres Arias wrote:
> > > As a side result, this shows that it now costs less than 100k USD to
> > > break cryptography with a security level of 64 bits (i.e. to compute
> > > 264 operations of symmetric cryptography).
>
> Just to clarify:
>
> As a stopgap measure, the collision-detection library of Stevens and Shumow [SS17]
> can be used to detect attack attempts (it successfully detects our attack).
>
> At the end of section 7.0,
And if anyone is curious, you can test your build of Git against their
sample files by running:
$ t/helper/test-tool sha1 <messageA
fatal: SHA-1 appears to be part of a collision attack: 8ac60ba76f1999a1ab70223f225aefdc78d4ddc0
Unfortunately you can't test with actual Git objects, because their
chosen-prefixes don't have object headers. They do estimate that a
classical collision is down to ~11k USD to compute, so maybe we'll see
one eventually. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] graph: fix collapse of multiple edges
From: Jeff King @ 2020-01-08 7:25 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, jcoglan, Derrick Stolee, Junio C Hamano
In-Reply-To: <12abb32531ed7125293986dc139a7ffed3839065.1578457675.git.gitgitgadget@gmail.com>
On Wed, Jan 08, 2020 at 04:27:55AM +0000, Derrick Stolee via GitGitGadget wrote:
> Before this change:
>
> | | | | | | *
> | |_|_|_|_|/|\
> |/| | | | |/ /
> | | | | |/| /
> | | | |/| |/
> | | |/| |/|
> | |/| |/| |
> | | |/| | |
>
> By adding the logic for "filling" a horizontal edge between the
> target column and the current column, we are able to resolve the
> issue.
>
> After this change:
>
> | | | | | | *
> | |_|_|_|_|/|\
> |/| | | | |/ /
> | | |_|_|/| /
> | |/| | | |/
> | | | |_|/|
> | | |/| | |
Hmm. Your description and your diagrams make sense to me. But one
curious thing is that the earlier test you added for 6_* does not need
modified. Because it continues to show:
| | | | * 6_F
| |_|_|/|
|/| | |/
| | |/|
| |/| |
| * | | 6_D
rather than adding a horizontal component to the second-parent line.
That seems inconsistent.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] unpack-trees: watch for out-of-range index position
From: Jeff King @ 2020-01-08 7:15 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
In-Reply-To: <20200108023127.219429-1-emilyshaffer@google.com>
On Tue, Jan 07, 2020 at 06:31:27PM -0800, Emily Shaffer wrote:
> This issue came in via a bugreport from a user who had done some nasty
> things like deleting various files in .git/ (and then couldn't remember
> how they had done it). The concern was primarily that a segfault is ugly
> and scary, and possibly dangerous; I didn't see much problem with
> checking for index-out-of-range if the result is a fatal error
> regardless.
>
> [...]
> if (pos >= 0)
> BUG("This is a directory and should not exist in index");
> pos = -pos - 1;
> - if (!starts_with(o->src_index->cache[pos]->name, name.buf) ||
> + if (pos >= o->src_index->cache_nr ||
> + !starts_with(o->src_index->cache[pos]->name, name.buf) ||
> (pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf)))
> - BUG("pos must point at the first entry in this directory");
> + BUG("pos %d doesn't point to the first entry of %s in index",
> + pos, name.buf);
The new condition you added looks correct to me. I suspect this BUG()
should not be a BUG() at all, though. It's not necessarily a logic error
inside Git, but as you showed it could indicate corrupt data we read
from disk. The true is probably same of the "pos >= 0" condition checked
above.
It's mostly an academic distinction, though, as I think it would be
pretty reasonable for now to just die() here (eventually, though, we
might want to turn it into an error return).
-Peff
^ permalink raw reply
* [PATCH] transport: don't flush when disconnecting stateless-rpc helper
From: Jeff King @ 2020-01-08 7:10 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jonathan Tan
In-Reply-To: <20200108033451.GN6570@camp.crustytoothpaste.net>
On Wed, Jan 08, 2020 at 03:34:51AM +0000, brian m. carlson wrote:
> A colleague (Jon Simons) today pointed out an interesting behavior of
> git ls-remote with protocol v2: it makes a second POST request and sends
> only a flush packet. This can be demonstrated with the following:
>
> GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin
>
> The Content-Length header on the second request will be exactly 4 bytes.
>
> I'm not sure exactly how to fix this by looking at remote-curl.c, but I
> suspect that we can avoid sending a request if all we're going to do is
> send a flush packet. If I were a bit more familiar with the code, I'd
> send a patch, but I'm not.
Yeah, in theory remote-curl could be smarter about turning such a
pointless request into a noop. But I think the root cause is actually on
the ls-remote side. How about this?
-- >8 --
Subject: transport: don't flush when disconnecting stateless-rpc helper
Since ba227857d2 (Reduce the number of connects when fetching,
2008-02-04), when we disconnect a git transport, we send a final flush
packet. This cleanly tells the other side that we're done, and avoids
the other side complaining "the remote end hung up unexpectedly" (though
we'd only see that for transports that pass along the server stderr,
like ssh or local-host).
But when we've initiated a v2 stateless-connect session over a transport
helper, there's no point in sending this flush packet. Each operation
we've performed is self-contained, and the other side is fine with us
hanging up between operations.
But much worse, by sending the flush packet we may cause the helper to
issue an entirely new request _just_ to send the flush packet. So we can
incur an extra network request just to say "by the way, we have nothing
more to send".
Let's drop this extra flush packet. As the test shows, this reduces the
number of POSTs required for a v2 ls-remote over http from 2 to 1.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5702-protocol-v2.sh | 12 ++++++++++++
transport.c | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index e73067d23f..7fd7102c87 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -665,6 +665,18 @@ test_expect_success 'fetch from namespaced repo respects namespaces' '
test_cmp expect actual
'
+test_expect_success 'ls-remote with v2 http sends only one POST' '
+ test_when_finished "rm -f log" &&
+
+ git ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >expect &&
+ GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \
+ ls-remote "$HTTPD_URL/smart/http_parent" >actual &&
+ test_cmp expect actual &&
+
+ grep "Send header: POST" log >posts &&
+ test_line_count = 1 posts
+'
+
test_expect_success 'push with http:// and a config of v2 does not request v2' '
test_when_finished "rm -f log" &&
# Till v2 for push is designed, make sure that if a client has
diff --git a/transport.c b/transport.c
index 83379a037d..1fdc7dac1a 100644
--- a/transport.c
+++ b/transport.c
@@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport)
{
struct git_transport_data *data = transport->data;
if (data->conn) {
- if (data->got_remote_heads)
+ if (data->got_remote_heads && !transport->stateless_rpc)
packet_flush(data->fd[1]);
close(data->fd[0]);
close(data->fd[1]);
--
2.25.0.rc1.618.gb996734ace
^ permalink raw reply related
* Re: [PATCH v2 00/17] merge: learn --autostash
From: Denton Liu @ 2020-01-08 6:08 UTC (permalink / raw)
To: phillip.wood
Cc: Junio C Hamano, Git Mailing List, Alban Gruin,
Johannes Schindelin
In-Reply-To: <4032e4bd-fa3d-54eb-fe95-38549dff3aaa@gmail.com>
Hi Phillip,
Sorry for the late reply. I'm back in school so I've been pretty busy
lately.
On Tue, Dec 31, 2019 at 10:34:30AM +0000, Phillip Wood wrote:
> For `merge --autostash` I think we need to consider the interaction of
> `--no-commit` with `--autostash` as `stash pop` will refuse to run if the
> merge modified any of the stashed paths.
The only time when we run the `stash pop` is when we either commit the
merge or abort it. With this in mind, what do you think of the following
test cases? If they look good, I can squash them into the appropriate
patch and send in a reroll.
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e0c8a0bad4..af5db58e16 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -727,6 +727,33 @@ test_expect_success 'conflicted merge with --autostash, --abort restores stash'
test_cmp file.1 file
'
+test_expect_success 'completed merge with --no-commit and --autostash' '
+ git reset --hard c1 &&
+ git merge-file file file.orig file.9 &&
+ git diff >expect &&
+ git merge --no-commit --autostash c2 &&
+ git stash show -p MERGE_AUTOSTASH >actual &&
+ test_cmp expect actual &&
+ git commit 2>err &&
+ test_i18ngrep "Applied autostash." err &&
+ git show HEAD:file >merge-result &&
+ test_cmp result.1-5 merge-result &&
+ test_cmp result.1-5-9 file
+'
+
+test_expect_success 'aborted merge with --no-commit and --autostash' '
+ git reset --hard c1 &&
+ git merge-file file file.orig file.9 &&
+ git diff >expect &&
+ git merge --no-commit --autostash c2 &&
+ git stash show -p MERGE_AUTOSTASH >actual &&
+ test_cmp expect actual &&
+ git merge --abort 2>err &&
+ test_i18ngrep "Applied autostash." err &&
+ git diff >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'merge with conflicted --autostash changes' '
git reset --hard c1 &&
git merge-file file file.orig file.9y &&
Thanks,
Denton
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* `git fetch origin ''` (the empty string) -- should this work?
From: Anthony Sottile @ 2020-01-08 5:28 UTC (permalink / raw)
To: Git Mailing List
this seems to end up doing the same as `git fetch origin HEAD` but I
can't figure out why -- perhaps someone on this list can help me :)
$ git fetch origin ''
From github.com:python/cpython
* branch HEAD -> FETCH_HEAD
what I expected was `fatal: Couldn't find remote ref ''` or something
of the sort as would be seen when fetching some non-existent branch:
$ git fetch origin wat
fatal: Couldn't find remote ref wat
Anthony
^ permalink raw reply
* [PATCH 2/2] graph: fix collapse of multiple edges
From: Derrick Stolee via GitGitGadget @ 2020-01-08 4:27 UTC (permalink / raw)
To: git; +Cc: peff, jcoglan, Derrick Stolee, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.518.git.1578457675.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
This fix resolves the previously-added test_expect_failure in
t4215-log-skewed-merges.sh.
The issue lies in the "else" condition while updating the mapping
inside graph_output_collapsing_line(). In 0f0f389f (graph: tidy up
display of left-skewed merges, 2019-10-15), the output of left-
skewed merges was changed to allow an immediate horizontal edge in
the first parent, output by graph_output_post_merge_line() instead
of by graph_output_collapsing_line(). This condensed the first line
behavior as follows:
Before 0f0f389f:
| | | | | | *-.
| | | | | | |\ \
| |_|_|_|_|/ | |
|/| | | | | / /
After 0f0f389f:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | | | |/| /
However, a very subtle issue arose when the second and third parent
edges are collapsed in later steps. The second parent edge is now
immediately adjacent to a vertical edge. This means that the
condition
} else if (graph->mapping[i - 1] < 0) {
in graph_output_collapsing_line() evaluates as false. The block for
this condition was the only place where we connected the target
column with the current position with horizontal edge markers.
In this case, the final "else" block is run, and the edge is marked
as horizontal, but did not back-fill the blank columns between the
target and the current edge. Since the second parent edge is marked
as horizontal, the third parent edge is not marked as horizontal.
This causes the output to continue as follows:
Before this change:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | | | |/| /
| | | |/| |/
| | |/| |/|
| |/| |/| |
| | |/| | |
By adding the logic for "filling" a horizontal edge between the
target column and the current column, we are able to resolve the
issue.
After this change:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | |_|_|/| /
| |/| | | |/
| | | |_|/|
| | |/| | |
This output properly matches the expected blend of the edge
behavior before 0f0f389f and the merge commit rendering from
0f0f389f.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
graph.c | 10 ++++++++--
t/t4215-log-skewed-merges.sh | 2 +-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/graph.c b/graph.c
index aaf97069bd..4fb25ad464 100644
--- a/graph.c
+++ b/graph.c
@@ -1233,8 +1233,14 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l
* prevent any other edges from moving
* horizontally.
*/
- if (horizontal_edge == -1)
- horizontal_edge = i;
+ if (horizontal_edge == -1) {
+ int j;
+ horizontal_edge_target = target;
+ horizontal_edge = i - 1;
+
+ for (j = (target * 2) + 3; j < (i - 2); j += 2)
+ graph->mapping[j] = target;
+ }
}
}
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 099e4b89b4..1d0d3240ff 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -311,7 +311,7 @@ test_expect_success 'log --graph with multiple tips and colors' '
test_cmp expect.colors actual.colors
'
-test_expect_failure 'log --graph with multiple tips' '
+test_expect_success 'log --graph with multiple tips' '
git checkout --orphan 7_1 &&
test_commit 7_A &&
test_commit 7_B &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] Graph horizontal fix
From: Derrick Stolee via GitGitGadget @ 2020-01-08 4:27 UTC (permalink / raw)
To: git; +Cc: peff, jcoglan, Derrick Stolee, Junio C Hamano
This depends on ds/graph-assert-fix.
This is a non-critical (not needed for v2.25.0) response to the previous
discussions [1] [2].
While working to resolve the fix for the assert() bug, I noticed this
regression when multiple edges wanted to collapse with horizontal lines. It
takes a reasonably large graph, but real projects are likely to demonstrate
this behavior.
I arranged the series into two patches: 1. the (failing) test, and 2. the
fix.
The fix commit includes some details about why the change to compress merge
commits caused this regression, and why I feel relatively confident that
this is a correct resolution.
Thanks, -Stolee
[1]
https://lore.kernel.org/git/faa954fa-ccb9-b034-a39d-d2f0696826ea@gmail.com/T/#t
[2]
https://lore.kernel.org/git/xmqqk1635gwu.fsf@gitster-ct.c.googlers.com/T/#t
Derrick Stolee (2):
graph: add test to demonstrate horizontal line bug
graph: fix collapse of multiple edges
graph.c | 10 ++++--
t/t4215-log-skewed-merges.sh | 62 ++++++++++++++++++++++++++++++++++++
2 files changed, 70 insertions(+), 2 deletions(-)
base-commit: aa2121af50498c7ea9d5c4c87f9dc66605bf772b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-518%2Fderrickstolee%2Fgraph-horizontal-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-518/derrickstolee/graph-horizontal-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/518
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/2] graph: add test to demonstrate horizontal line bug
From: Derrick Stolee via GitGitGadget @ 2020-01-08 4:27 UTC (permalink / raw)
To: git; +Cc: peff, jcoglan, Derrick Stolee, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.518.git.1578457675.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
A previous test in t4215-log-skewed-merges.sh was added to demonstrate
exactly the topology of a reported failure in "git log --graph". While
investigating the fix, we realized that multiple edges that could
collapse with horizontal lines were not doing so.
Specifically, examine this section of the graph:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | | | |/| /
| | | |/| |/
| | |/| |/|
| |/| |/| |
| | |/| | |
| | * | | |
Document this behavior with a test. This behavior is new, as the
behavior in v2.24.1 has the following output:
| | | | | | *-.
| | | | | | |\ \
| |_|_|_|_|/ / /
|/| | | | | / /
| | |_|_|_|/ /
| |/| | | | /
| | | |_|_|/
| | |/| | |
| | * | | |
The behavior changed logically in 479db18b ("graph: smooth appearance
of collapsing edges on commit lines", 2019-10-15), but was actually
broken due to an assert() bug in 458152cc ("graph: example of graph
output that can be simplified", 2019-10-15). A future change could
modify this behavior to do the following instead:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | |_|_|/| /
| |/| | | |/
| | | |_|/|
| | |/| | |
| | * | | |
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
t/t4215-log-skewed-merges.sh | 62 ++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 5661ed5881..099e4b89b4 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -311,4 +311,66 @@ test_expect_success 'log --graph with multiple tips and colors' '
test_cmp expect.colors actual.colors
'
+test_expect_failure 'log --graph with multiple tips' '
+ git checkout --orphan 7_1 &&
+ test_commit 7_A &&
+ test_commit 7_B &&
+ test_commit 7_C &&
+ git checkout -b 7_2 7_1~2 &&
+ test_commit 7_D &&
+ test_commit 7_E &&
+ git checkout -b 7_3 7_1~1 &&
+ test_commit 7_F &&
+ test_commit 7_G &&
+ git checkout -b 7_4 7_2~1 &&
+ test_commit 7_H &&
+ git checkout -b 7_5 7_1~2 &&
+ test_commit 7_I &&
+ git checkout -b 7_6 7_3~1 &&
+ test_commit 7_J &&
+ git checkout -b M_1 7_1 &&
+ git merge --no-ff 7_2 -m 7_M1 &&
+ git checkout -b M_3 7_3 &&
+ git merge --no-ff 7_4 -m 7_M2 &&
+ git checkout -b M_5 7_5 &&
+ git merge --no-ff 7_6 -m 7_M3 &&
+ git checkout -b M_7 7_1 &&
+ git merge --no-ff 7_2 7_3 -m 7_M4 &&
+
+ check_graph M_1 M_3 M_5 M_7 <<-\EOF
+ * 7_M1
+ |\
+ | | * 7_M2
+ | | |\
+ | | | * 7_H
+ | | | | * 7_M3
+ | | | | |\
+ | | | | | * 7_J
+ | | | | * | 7_I
+ | | | | | | * 7_M4
+ | |_|_|_|_|/|\
+ |/| | | | |/ /
+ | | |_|_|/| /
+ | |/| | | |/
+ | | | |_|/|
+ | | |/| | |
+ | | * | | | 7_G
+ | | | |_|/
+ | | |/| |
+ | | * | | 7_F
+ | * | | | 7_E
+ | | |/ /
+ | |/| |
+ | * | | 7_D
+ | | |/
+ | |/|
+ * | | 7_C
+ | |/
+ |/|
+ * | 7_B
+ |/
+ * 7_A
+ EOF
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* Extra request with protocol v2 and git ls-remote
From: brian m. carlson @ 2020-01-08 3:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
[-- Attachment #1: Type: text/plain, Size: 751 bytes --]
A colleague (Jon Simons) today pointed out an interesting behavior of
git ls-remote with protocol v2: it makes a second POST request and sends
only a flush packet. This can be demonstrated with the following:
GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin
The Content-Length header on the second request will be exactly 4 bytes.
I'm not sure exactly how to fix this by looking at remote-curl.c, but I
suspect that we can avoid sending a request if all we're going to do is
send a flush packet. If I were a bit more familiar with the code, I'd
send a patch, but I'm not.
I just thought I'd point this out since it was surprising to me.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 0/1] sequencer: comment out the 'squash!' line
From: brian m. carlson @ 2020-01-08 2:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mike Rappazzo, Michael Rappazzo via GitGitGadget, Git List
In-Reply-To: <xmqqlfqj6y5n.fsf@gitster-ct.c.googlers.com>
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On 2020-01-07 at 16:15:16, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > I can see the argument that this makes it a little harder for mechanical
> > processing across versions, but I suspect most of that looks something
> > like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be
> > affected.
>
> With the left-anchoring, the search pattern will no longer find that
> line if "squash!" is commented out, but people tend to be sloppy and
> do not anchor the pattern would not notice the difference. Perhaps
> the downside may not be too severe? I dunno.
Sorry, I was perhaps bad at explaining this. In my example, it would no
longer remove that line, but the user wouldn't care, because it would be
commented out and removed automatically. So while the code wouldn't
work, what the user wanted would be done by Git automatically.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: brian m. carlson @ 2020-01-08 2:47 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Miriam R.
In-Reply-To: <20200107110145.GA1073219@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 638 bytes --]
On 2020-01-07 at 11:01:45, Jeff King wrote:
> > Noticed-by: Miriam R. <mirucam@gmail.com>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>
> I think Miriam actually posted the same patch in her initial email:
>
> https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/
>
> I don't know how we want to handle authorship.
I don't feel strongly about holding authorship; I'm happy to have her
name on it instead of mine since she did propose a solution. I just
care that we fix it.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* [RFC PATCH] unpack-trees: watch for out-of-range index position
From: Emily Shaffer @ 2020-01-08 2:31 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
It's possible in a case where the index file contains a tree extension
but no blobs within that tree exist for index_pos_by_traverse_info() to
segfault. If the name_entry passed into index_pos_by_traverse_info() has
no blobs inside, AND is alphabetically later than all blobs currently in
the index file, index_pos_by_traverse_info() will segfault. For example,
an index file which looks something like this:
aaa#0
bbb/aaa#0
[Extensions]
TREE: zzz
In this example, 'index_name_pos(..., "zzz/", ...)' will return '-4',
indicating that "zzz/" could be inserted at position 3. However, when
the checks which ensure that the insertion position of "zzz/" look for a
blob at that position beginning with "zzz/", the index cache is accessed
out of range, causing a segfault.
This kind of index state is not typically generated during user
operations, and is in fact an edge case of the state being checked for
in the conditional where it was added. However, since the entry for the
BUG() line is ambiguous, tell some additional context to help Git
developers debug the failure later. When we know the name of the dir we
were trying to look up, it becomes possible to examine the index file
in a hex util to determine what went wrong; the position gives a hint
about where to start looking.
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
This issue came in via a bugreport from a user who had done some nasty
things like deleting various files in .git/ (and then couldn't remember
how they had done it). The concern was primarily that a segfault is ugly
and scary, and possibly dangerous; I didn't see much problem with
checking for index-out-of-range if the result is a fatal error
regardless.
The index file they shared for reproduction was very similar to the one
that I proposed in the commit message. However, though I had a repo
where I could reproduce, since the user wasn't sure how they had gotten
there I struggled reasoning about how to produce these exact conditions.
It seems like during normal operation the index shouldn't learn about a
tree extension where it doesn't know any blobs (in fact, I've become
irritated before about being unable to stage/commit only directory
structure :) ).
I also didn't find any test cases looking for the BUG() as it exists
now; I guess that's because BUG()s are supposed to be unreachable during
normal operation (or else they'd be a die()). So, it's marked RFC only
because I couldn't think of a way to reliably reproduce or test this
change.
- Emily
unpack-trees.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index a75b068d33..d1ac1e179c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -694,9 +694,11 @@ static int index_pos_by_traverse_info(struct name_entry *names,
if (pos >= 0)
BUG("This is a directory and should not exist in index");
pos = -pos - 1;
- if (!starts_with(o->src_index->cache[pos]->name, name.buf) ||
+ if (pos >= o->src_index->cache_nr ||
+ !starts_with(o->src_index->cache[pos]->name, name.buf) ||
(pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf)))
- BUG("pos must point at the first entry in this directory");
+ BUG("pos %d doesn't point to the first entry of %s in index",
+ pos, name.buf);
strbuf_release(&name);
return pos;
}
--
2.25.0.rc1.283.g88dfdc4193-goog
^ 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