* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
From: Emily Shaffer @ 2020-01-08 1:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Eric Sunshine, Heba Waly via GitGitGadget, Git List, Heba Waly
In-Reply-To: <xmqqh8176xab.fsf@gitster-ct.c.googlers.com>
On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> [jc: skipped all the good suggestions I agree with]
>
> >> + }
> >> + else {
> >> + advise(_("The branch you are trying to delete is checked "
> >> + "out on another worktree, run the following command "
> >> + "to checkout a different branch then try again:\n"
> >> + "git -C %s switch <branch>"), wt->path);
> >
> > I like the use of -C here because it makes the command self-contained,
> > however, I also worry because wt->path is an absolute path, thus
> > likely to be quite lengthy, which means that the important part of the
> > example command (the "switch <branch>") can get pushed quite far away,
> > thus is more easily overlooked by the reader. I wonder if it would
> > make more sense to show the 'cd' command explicitly, although doing so
> > ties the example to a particular shell, which may be a downside.
> >
> > cd %s
> > git switch <different-branch>
> > cd -
> > git branch -%c <this-branch>
>
> Note that wt->path may have special characters that would need to be
> protected from the user's shell (worse, the quoting convention may
> be different depending on which shell is in use). That is one of
> the reasons why I would suggest to stay away from giving an advice
> that pretends to be cut-and-paste-able without being so.
Hm, I think you've sold me on the error of my ways trying to push for
copy-pasteable advices :)
But I wonder, how much is too much? I mean that suggesting a single Git
command which takes a branch name and a pathspec is safer than
suggesting some complicated -C=foo or cd bar thing, right?
> In this
> case, <different-branch> and <this-branch> must be filled by the
> user anyway, and the only thing worth cutting-and-pasting is the
> path to the other worktree, not the "git -C" or "cd" that users
> should be able to come up with.
>
> "The branch is checked out on another worktree at\n"
> "path '%s'\n"
> "and cannot be deleted. Go there, check out some other\n"
> "branch and try again."
>
> or something like that, perhaps?
>
> > (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. 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, and to deal with
the other requires web searching, asking people, reading documentation,
perhaps gaining "tips" from questionable blogs which don't actually give
very useful advice. I think we were just discussing not long ago the
general quality of advice on StackOverflow, for example.
- Emily
^ permalink raw reply
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
From: Heba Waly @ 2020-01-08 1:14 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano
In-Reply-To: <CAPig+cQ0qY8KDZrQ8khuz34DqPimorN7JHHn0Ms=KpvJYtxJoA@mail.gmail.com>
On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Jan 6, 2020 at 11:10 PM Heba Waly via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > branch: advise the user to checkout a different branch before deleting
> >
> > Display a hint to the user when attempting to delete a checked out
> > branch.
> >
> > Currently the user gets an error message saying: "error: Cannot delete
> > branch <branch> checked out at <path>". The hint will be displayed
> > after the error message.
>
> A couple comments...
>
> The second paragraph doesn't say anything beyond what the patch/code
> itself already says clearly (plus, there's no need to state the
> obvious), so the paragraph adds no value (yet eats up reviewer time).
> Therefore, it can be dropped.
>
> To convince readers that the change made by the patch is indeed
> warranted, it's always important to explain _why_ this change is being
> made.
>
> Both points can be addressed with a short and sweet commit message,
> perhaps like this:
>
> branch: advise how to delete checked-out branch
>
> Teach newcomers how to deal with Git refusing to delete a
> checked-out branch (whether in the current worktree or some
> other).
>
Agree, thanks.
> 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.
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> > diff --git a/advice.c b/advice.c
> > @@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;
> > @@ -91,7 +92,8 @@ static struct {
> > { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
> > -
> > + { "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
> > +
>
> When you see an odd-looking diff like this in which you wouldn't
> expect any diff markers on the blank line (that is, the blank line got
> deleted and re-added), it's a good indication that there's unwanted
> trailing whitespace on one of the lines. In this case, you (or more
> likely your editor automatically) added trailing whitespace to the
> blank line which doesn't belong there. Unwanted whitespace changes
> like this make the patch noisier and more difficult for a reviewer to
> read.
>
Mystery solved! thanks.
> > diff --git a/advice.h b/advice.h
> > @@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;
> > +extern int advice_delete_checkedout_branch;
>
> Is there precedent elsewhere for spelling this "checkedout" rather
> than the more natural "checked_out"?
>
Not really.
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> > + if (advice_delete_checkedout_branch) {
> > + if (wt->is_current) {
> > + advise(_("The branch you are trying to delete is already "
> > + "checked out, run the following command to "
> > + "checkout a different branch then try again:\n"
> > + "git switch <branch>"));
>
> This advice unnecessarily repeats what the error message just above it
> already says about the branch being checked out (thus adds no value),
> and then jumps directly into showing the user an opaque command to
> resolve the situation without explaining _how_ or _why_ the command is
> supposed to help.
>
> Advice messages elsewhere typically indent the example command to make
> it stand out from the explanatory prose (and often separated it from
> the text by a blank line).
>
> A rewrite which addresses both these issues might be something like:
>
> Switch to a different branch before trying to delete it. For
> example:
>
> git switch <different-branch>
> git branch -%c <this-branch>
>
> (and fill in %c with either "-d" or "-D" depending upon the value of 'force')
Ok.
> > + }
> > + else {
> > + advise(_("The branch you are trying to delete is checked "
> > + "out on another worktree, run the following command "
> > + "to checkout a different branch then try again:\n"
> > + "git -C %s switch <branch>"), wt->path);
>
> I like the use of -C here because it makes the command self-contained,
> however, I also worry because wt->path is an absolute path, thus
> likely to be quite lengthy, which means that the important part of the
> example command (the "switch <branch>") can get pushed quite far away,
> thus is more easily overlooked by the reader. I wonder if it would
> make more sense to show the 'cd' command explicitly, although doing so
> ties the example to a particular shell, which may be a downside.
>
> cd %s
> git switch <different-branch>
> cd -
> git branch -%c <this-branch>
>
> (It is rather verbose and ugly, though.)
>
> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> > @@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '
> > test_expect_success 'deleting currently checked out branch fails' '
> > git worktree add -b my7 my7 &&
> > - test_must_fail git -C my7 branch -d my7 &&
> > - test_must_fail git branch -d my7 &&
> > + test_must_fail git -C my7 branch -d my7 2>output1.err &&
> > + test_must_fail git branch -d my7 2>output2.err &&
> > + test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err &&
> > + test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err &&
>
> Nit: Separating the 'grep' from the command which generated the error
> output makes it harder for a reader to see at a glance what is being
> tested and to reason about it since it demands that the reader keep
> two distinct cases in mind rather than merely focusing on one at a
> time. Also, doing it this way forces you to invent distinct filenames
> (by appending a number, for instance), which further leads the reader
> to wonder if there is some significance (later in the test) to keeping
> these outputs in separate files. So, a better organization (with more
> natural filenames) would be:
>
> test_must_fail git -C my7 branch -d my7 2>output.err &&
> test_i18ngrep "hint: ..." output.err &&
> test_must_fail git branch -d my7 2>output.err &&
> test_i18ngrep "hint: ..." output.err &&
Agree.
Thanks Eric, I'll not update this patch unless somebody thinks it's
adding a value and worth spending more time on it.
Heba
^ permalink raw reply
* Re: [PATCH 6/9] commit-graph: test commit-graph write --changed-paths
From: Jakub Narebski @ 2020-01-08 0:32 UTC (permalink / raw)
To: Garima Singh via GitGitGadget
Cc: git, Derrick Stolee, SZEDER Gábor, Jonathan Tan,
Jeff Hostetler, Taylor Blau, Jeff King, Junio C Hamano,
Garima Singh
In-Reply-To: <85bfdfa59c48891343e3eeb740a4b3554405909a.1576879520.git.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Garima Singh <garima.singh@microsoft.com>
>
> Add tests for the --changed-paths feature when writing
> commit-graphs.
It doesn't look however as if this test is actually testing the _Bloom
filter_ functionality itself -- because this test looks like
copy'n'paste of t/t5324-split-commit-graph.sh, just with
`--changed-paths` added to the `git commit-graph write` invocation, and
added checking via enhanced test-tool that there are Bloom filter chunks
("bloom_indexes" and "bloom_data").
Please correct me if I am wrong, but this looks like a simple sanity
check for me.
>
> RFC Notes:
> I plan to split this test across some of the earlier commits
> as appropriate.
About adding tests to earlier commits in this series:
1. Testing Bloom filter functionality:
- creating Bloom filter and adding elements to it
- testing Bloom filter functionality
- for element in set the answer is "maybe"
- for element not in set the answer is "no" or "maybe"
- automatic resizing works (6 and 7 elements)
- it works for different number of hash functions,
and different number of bits per element (maybe?)
2. Testing Bloom filter for commit changeset:
- it works for commit with no changes
- it works for merge commit with no changes to first parent
(`git merge --strategy=ours`)
- with number of changes that require filter size change
- with maximal number of changes, one changed file less,
one changed file more
- that for file deeper in hierarchy, path/to/file, all of
changed directories (path/to/ and path/) are also added
3. Test writing and reading commit-graph with Bloom filters
- that after writing Bloom filters with `--changed-paths`
the data is present in commit-graph files
- it works correctly with split commit-graph
- it doesn't crash if confronted with unknown settings:
hash version different than 1, different number of hash
functions, different number of bits per element
4. Bloom filter specific `git commit-graph verify` parts
- fail if Bloom filter chunks appear multiple times
- fail if only one of BIDX or BDAT chunks are present
- fail if BIDX is not monotonic, that is if size of Bloom filter
for a commit is negative
- fail if BDAT size does not agree with BIDX,
being either too small, or too large
- check if values of number of hash functions
and number of bits per element added are sane
5. Using Bloom filters to speed up Git operations
- test that with and without Bloom filters (or commit-graph)
the following operations work the same:
- git log -- <path/to/file>
- git log -- <path/to/directory>
- git log -- '*.c' # or other glob pattern
- git log -- <file1> <file2>
- git log --follow <file>
- maybe also `git log --full-history -- <file>`
- if possible, add performance tests, see `t/perf`
>
> Helped-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> t/helper/test-read-graph.c | 4 +
> t/t5325-commit-graph-bloom.sh | 255 ++++++++++++++++++++++++++++++++++
> 2 files changed, 259 insertions(+)
> create mode 100755 t/t5325-commit-graph-bloom.sh
>
> diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
> index d2884efe0a..aff597c7a3 100644
> --- a/t/helper/test-read-graph.c
> +++ b/t/helper/test-read-graph.c
> @@ -45,6 +45,10 @@ int cmd__read_graph(int argc, const char **argv)
> printf(" commit_metadata");
> if (graph->chunk_extra_edges)
> printf(" extra_edges");
> + if (graph->chunk_bloom_indexes)
> + printf(" bloom_indexes");
> + if (graph->chunk_bloom_data)
> + printf(" bloom_data");
All right, though it is very basic information.
> printf("\n");
>
> UNLEAK(graph);
> diff --git a/t/t5325-commit-graph-bloom.sh b/t/t5325-commit-graph-bloom.sh
> new file mode 100755
> index 0000000000..d7ef0e7fb3
> --- /dev/null
> +++ b/t/t5325-commit-graph-bloom.sh
> @@ -0,0 +1,255 @@
> +#!/bin/sh
> +
> +test_description='commit graph with bloom filters'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup repo' '
> + git init &&
> + git config core.commitGraph true &&
> + git config gc.writeCommitGraph false &&
> + infodir=".git/objects/info" &&
> + graphdir="$infodir/commit-graphs" &&
> + test_oid_init
> +'
> +
> +graph_read_expect() {
Style: space between function name and parentheses, i.e.
+graph_read_expect () {
> + OPTIONAL=""
Not used anywhere.
> + NUM_CHUNKS=5
> + if test ! -z $2
It might be good idea to add names to those parameters by setting some
local variables to $1 and $2; or, alternatively add comment describing
this function.
> + then
> + OPTIONAL=" $2"
> + NUM_CHUNKS=$((NUM_CHUNKS + $(echo "$2" | wc -w)))
> + fi
> + cat >expect <<- EOF
> + header: 43475048 1 1 $NUM_CHUNKS 0
> + num_commits: $1
> + chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
> + EOF
> + test-tool read-graph >output &&
> + test_cmp expect output
> +}
No comments below this point...
Best,
Jakub Narębski
> +
> +test_expect_success 'create commits and write commit-graph' '
> + for i in $(test_seq 3)
> + do
> + test_commit $i &&
> + git branch commits/$i || return 1
> + done &&
> + git commit-graph write --reachable --changed-paths &&
> + test_path_is_file $infodir/commit-graph &&
> + graph_read_expect 3
> +'
> +
> +graph_git_two_modes() {
> + git -c core.commitGraph=true $1 >output
> + git -c core.commitGraph=false $1 >expect
> + test_cmp expect output
> +}
> +
> +graph_git_behavior() {
> + MSG=$1
> + BRANCH=$2
> + COMPARE=$3
> + test_expect_success "check normal git operations: $MSG" '
> + graph_git_two_modes "log --oneline $BRANCH" &&
> + graph_git_two_modes "log --topo-order $BRANCH" &&
> + graph_git_two_modes "log --graph $COMPARE..$BRANCH" &&
> + graph_git_two_modes "branch -vv" &&
> + graph_git_two_modes "merge-base -a $BRANCH $COMPARE"
> + '
> +}
> +
> +graph_git_behavior 'graph exists' commits/3 commits/1
> +
> +verify_chain_files_exist() {
> + for hash in $(cat $1/commit-graph-chain)
> + do
> + test_path_is_file $1/graph-$hash.graph || return 1
> + done
> +}
> +
> +test_expect_success 'add more commits, and write a new base graph' '
> + git reset --hard commits/1 &&
> + for i in $(test_seq 4 5)
> + do
> + test_commit $i &&
> + git branch commits/$i || return 1
> + done &&
> + git reset --hard commits/2 &&
> + for i in $(test_seq 6 10)
> + do
> + test_commit $i &&
> + git branch commits/$i || return 1
> + done &&
> + git reset --hard commits/2 &&
> + git merge commits/4 &&
> + git branch merge/1 &&
> + git reset --hard commits/4 &&
> + git merge commits/6 &&
> + git branch merge/2 &&
> + git commit-graph write --reachable --changed-paths &&
> + graph_read_expect 12
> +'
> +
> +test_expect_success 'fork and fail to base a chain on a commit-graph file' '
> + test_when_finished rm -rf fork &&
> + git clone . fork &&
> + (
> + cd fork &&
> + rm .git/objects/info/commit-graph &&
> + echo "$(pwd)/../.git/objects" >.git/objects/info/alternates &&
> + test_commit new-commit &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_path_is_file $graphdir/commit-graph-chain &&
> + test_line_count = 1 $graphdir/commit-graph-chain &&
> + verify_chain_files_exist $graphdir
> + )
> +'
> +
> +test_expect_success 'add three more commits, write a tip graph' '
> + git reset --hard commits/3 &&
> + git merge merge/1 &&
> + git merge commits/5 &&
> + git merge merge/2 &&
> + git branch merge/3 &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_path_is_missing $infodir/commit-graph &&
> + test_path_is_file $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 2 graph-files &&
> + verify_chain_files_exist $graphdir
> +'
> +
> +graph_git_behavior 'split commit-graph: merge 3 vs 2' merge/3 merge/2
> +
> +test_expect_success 'add one commit, write a tip graph' '
> + test_commit 11 &&
> + git branch commits/11 &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_path_is_missing $infodir/commit-graph &&
> + test_path_is_file $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 3 graph-files &&
> + verify_chain_files_exist $graphdir
> +'
> +
> +graph_git_behavior 'three-layer commit-graph: commit 11 vs 6' commits/11 commits/6
> +
> +test_expect_success 'add one commit, write a merged graph' '
> + test_commit 12 &&
> + git branch commits/12 &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_path_is_file $graphdir/commit-graph-chain &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 2 graph-files &&
> + verify_chain_files_exist $graphdir
> +'
> +
> +graph_git_behavior 'merged commit-graph: commit 12 vs 6' commits/12 commits/6
> +
> +test_expect_success 'create fork and chain across alternate' '
> + git clone . fork &&
> + (
> + cd fork &&
> + git config core.commitGraph true &&
> + rm -rf $graphdir &&
> + echo "$(pwd)/../.git/objects" >.git/objects/info/alternates &&
> + test_commit 13 &&
> + git branch commits/13 &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_path_is_file $graphdir/commit-graph-chain &&
> + test_line_count = 3 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 1 graph-files &&
> + git -c core.commitGraph=true rev-list HEAD >expect &&
> + git -c core.commitGraph=false rev-list HEAD >actual &&
> + test_cmp expect actual &&
> + test_commit 14 &&
> + git commit-graph write --reachable --split --changed-paths --object-dir=.git/objects/ &&
> + test_line_count = 3 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 1 graph-files
> + )
> +'
> +
> +graph_git_behavior 'alternate: commit 13 vs 6' commits/13 commits/6
> +
> +test_expect_success 'test merge stragety constants' '
> + git clone . merge-2 &&
> + (
> + cd merge-2 &&
> + git config core.commitGraph true &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + test_commit 14 &&
> + git commit-graph write --reachable --split --changed-paths --size-multiple=2 &&
> + test_line_count = 3 $graphdir/commit-graph-chain
> +
> + ) &&
> + git clone . merge-10 &&
> + (
> + cd merge-10 &&
> + git config core.commitGraph true &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + test_commit 14 &&
> + git commit-graph write --reachable --split --changed-paths --size-multiple=10 &&
> + test_line_count = 1 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 1 graph-files
> + ) &&
> + git clone . merge-10-expire &&
> + (
> + cd merge-10-expire &&
> + git config core.commitGraph true &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + test_commit 15 &&
> + git commit-graph write --reachable --split --changed-paths --size-multiple=10 --expire-time=1980-01-01 &&
> + test_line_count = 1 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 3 graph-files
> + ) &&
> + git clone --no-hardlinks . max-commits &&
> + (
> + cd max-commits &&
> + git config core.commitGraph true &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + test_commit 16 &&
> + test_commit 17 &&
> + git commit-graph write --reachable --split --changed-paths --max-commits=1 &&
> + test_line_count = 1 $graphdir/commit-graph-chain &&
> + ls $graphdir/graph-*.graph >graph-files &&
> + test_line_count = 1 graph-files
> + )
> +'
> +
> +test_expect_success 'remove commit-graph-chain file after flattening' '
> + git clone . flatten &&
> + (
> + cd flatten &&
> + test_line_count = 2 $graphdir/commit-graph-chain &&
> + git commit-graph write --reachable &&
> + test_path_is_missing $graphdir/commit-graph-chain &&
> + ls $graphdir >graph-files &&
> + test_must_be_empty graph-files
> + )
> +'
> +
> +graph_git_behavior 'graph exists' merge/octopus commits/12
> +
> +test_expect_success 'split across alternate where alternate is not split' '
> + git commit-graph write --reachable &&
> + test_path_is_file .git/objects/info/commit-graph &&
> + cp .git/objects/info/commit-graph . &&
> + git clone --no-hardlinks . alt-split &&
> + (
> + cd alt-split &&
> + rm -f .git/objects/info/commit-graph &&
> + echo "$(pwd)"/../.git/objects >.git/objects/info/alternates &&
> + test_commit 18 &&
> + git commit-graph write --reachable --split --changed-paths &&
> + test_line_count = 1 $graphdir/commit-graph-chain
> + ) &&
> + test_cmp commit-graph .git/objects/info/commit-graph
> +'
> +
> +test_done
^ permalink raw reply
* [PATCH v2 1/1] doc/gitcore-tutorial: fix prose to match example command
From: Heba Waly via GitGitGadget @ 2020-01-08 0:31 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano, Heba Waly
In-Reply-To: <pull.515.v2.git.1578443496.gitgitgadget@gmail.com>
From: Heba Waly <heba.waly@gmail.com>
In 328c6cb853 (doc: promote "git switch", 2019-03-29), an example
was changed to use "git switch" rather than "git checkout" but an
instance of "git checkout" in the explanatory text preceding the
example was overlooked. Fix this oversight.
Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index f880d21dfb..c0b95256cc 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -751,7 +751,7 @@ to it.
================================================
If you make the decision to start your new branch at some
other point in the history than the current `HEAD`, you can do so by
-just telling 'git checkout' what the base of the checkout would be.
+just telling 'git switch' what the base of the checkout would be.
In other words, if you have an earlier tag or branch, you'd just do
------------
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/1] [Outreachy] doc/gitcore-tutorial: fix prose to match example command
From: Heba Waly via GitGitGadget @ 2020-01-08 0:31 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano
In-Reply-To: <pull.515.git.1578391553.gitgitgadget@gmail.com>
In 328c6cb853 (doc: promote "git switch", 2019-03-29), an example was
changed to use "git switch" rather than "git checkout" but an instance of
"git checkout" in the explanatory text preceding the example was overlooked.
Fix this oversight.
Heba Waly (1):
doc/gitcore-tutorial: fix prose to match example command
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-515%2FHebaWaly%2Fgitcore_tutorial_typo-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-515/HebaWaly/gitcore_tutorial_typo-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/515
Range-diff vs v1:
1: ee8636e0ed ! 1: 0c75cd8f97 doc: fix a typo in gitcore-tutorial.txt
@@ -1,6 +1,11 @@
Author: Heba Waly <heba.waly@gmail.com>
- doc: fix a typo in gitcore-tutorial.txt
+ doc/gitcore-tutorial: fix prose to match example command
+
+ In 328c6cb853 (doc: promote "git switch", 2019-03-29), an example
+ was changed to use "git switch" rather than "git checkout" but an
+ instance of "git checkout" in the explanatory text preceding the
+ example was overlooked. Fix this oversight.
Signed-off-by: Heba Waly <heba.waly@gmail.com>
--
gitgitgadget
^ permalink raw reply
* [PATCH] submodule add: show 'add --dry-run' stderr when aborting
From: Kyle Meyer @ 2020-01-08 0:31 UTC (permalink / raw)
To: git; +Cc: Kyle Meyer, Yaroslav O Halchenko
Unless --force is specified, 'submodule add' checks if the destination
path is ignored by calling 'git add --dry-run --ignore-missing', and,
if that call fails, aborts with a custom "path is ignored" message (a
slight variant of what 'git add' shows). Aborting early rather than
letting the downstream 'git add' call fail is done so that the command
exits before cloning into the destination path. However, in rare
cases where the dry-run call fails for a reason other than the path
being ignored---for example, due to a preexisting index.lock
file---displaying the "ignored path" error message hides the real
source of the failure.
Instead of displaying the tailored "ignored path" message, let's
report the standard error from the dry run to give the caller more
accurate information about failures that are not due to an ignored
path.
For the ignored path case, this leads to the following change in the
error message:
The following [-path is-]{+paths are+} ignored by one of your .gitignore files:
<destination path>
Use -f if you really want to add [-it.-]{+them.+}
The new phrasing is a bit awkward, because 'submodule add' is only
dealing with one destination path. Alternatively, we could continue
to use the tailored message when the exit code is 1 (the expected
status for a failure due to an ignored path) and relay the standard
error for all other non-zero exits. That, however, risks hiding the
message of unrelated failures that share an exit code of 1, so it
doesn't seem worth doing just to avoid a clunkier, but still clear,
error message.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
git-submodule.sh | 14 ++++++++------
t/t7400-submodule-basic.sh | 15 +++++++++++++--
2 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index aaa1809d24..afcb4c0948 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,13 +241,15 @@ cmd_add()
die "$(eval_gettext "'\$sm_path' does not have a commit checked out")"
fi
- if test -z "$force" &&
- ! git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" > /dev/null 2>&1
+ if test -z "$force"
then
- eval_gettextln "The following path is ignored by one of your .gitignore files:
-\$sm_path
-Use -f if you really want to add it." >&2
- exit 1
+ dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" 2>&1 >/dev/null)
+ res=$?
+ if test $res -ne 0
+ then
+ echo >&2 "$dryerr"
+ exit $res
+ fi
fi
if test -n "$custom_name"
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 7f75bb1be6..42a00f95b9 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -156,9 +156,9 @@ test_expect_success 'submodule add to .gitignored path fails' '
(
cd addtest-ignore &&
cat <<-\EOF >expect &&
- The following path is ignored by one of your .gitignore files:
+ The following paths are ignored by one of your .gitignore files:
submod
- Use -f if you really want to add it.
+ Use -f if you really want to add them.
EOF
# Does not use test_commit due to the ignore
echo "*" > .gitignore &&
@@ -191,6 +191,17 @@ test_expect_success 'submodule add to reconfigure existing submodule with --forc
)
'
+test_expect_success 'submodule add relays add --dry-run stderr' '
+ test_when_finished "rm -rf addtest/.git/index.lock" &&
+ (
+ cd addtest &&
+ : >.git/index.lock &&
+ ! git submodule add "$submodurl" sub-while-locked 2>output.err &&
+ test_i18ngrep "^fatal: .*index\.lock" output.err &&
+ test_path_is_missing sub-while-locked
+ )
+'
+
test_expect_success 'submodule add --branch' '
echo "refs/heads/initial" >expect-head &&
cat <<-\EOF >expect-heads &&
base-commit: 042ed3e048af08014487d19196984347e3be7d1c
--
2.24.1
^ permalink raw reply related
* Re: [PATCH 1/1] doc: fix a typo in gitcore-tutorial.txt
From: Heba Waly @ 2020-01-08 0:09 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano
In-Reply-To: <CAPig+cTv5SOxEjjVQ7QvqJ1WbZGbcXegcCP4d5CK+nSdJvkNdQ@mail.gmail.com>
On Wed, Jan 8, 2020 at 12:35 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
>
> Calling this change a "typo fix" confuses reviewers since it's clearly
> not a mere typographical error. It looks instead as if you are
> recommending git-switch over git-checkout, so a reader would expect
> the commit message to justify that change rather than merely calling
> it a "typo fix". However, digging deeper, one finds that this is
> actually fixing an oversight from an earlier change which already
> updated this file to prefer git-switch over git-checkout.
>
> To save reviewers the time and effort of having to figure all this
> out, use the commit message to explain the situation. For example, you
> might say:
>
> doc/gitcore-tutorial: fix prose to match example command
>
> In 328c6cb853 (doc: promote "git switch", 2019-03-29), an example
> was changed to use "git switch" rather than "git checkout" but an
> instance of "git checkout" in the explanatory text preceding the
> example was overlooked. Fix this oversight.
Looks like I overlooked this commit for its simplicity, I thought it
was too simple and self explanatory. I agree with you though.
Thanks,
Heba
^ permalink raw reply
* Re: [PATCH 1/1] add: use advise function to display hints
From: Heba Waly @ 2020-01-07 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Heba Waly via GitGitGadget
In-Reply-To: <xmqqd0bv6x79.fsf@gitster-ct.c.googlers.com>
On Wed, Jan 8, 2020 at 5:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Heba Waly <heba.waly@gmail.com> writes:
>
> > On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> > That's a valid suggestion, I can investigate that in a new patch, I'd rather
> > keep this one as simple as calling the existing advise function.
>
> Yeah, the side note wasn't even a suggestion for improving _this_
> topic, nor even specifically addressed to you. Let's stay focused.
>
Sending out this patch, not only do I want the community's say in using advise()
in add.c but using it in general in more locations where hints are
displayed using
printf() or any other similar function. So when you pointed out that
it's not supposed
to be used without checking the corresponding configuration variable,
I had a similar
thought to yours, that it can be improved.
Accordingly I might be interested in looking in to this once I finish
what I have in hand.
Thanks,
Heba
^ permalink raw reply
* [PATCH v2 1/1] add: use advise function to display hints
From: Heba Waly via GitGitGadget @ 2020-01-07 23:12 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano, Heba Waly
In-Reply-To: <pull.508.v2.git.1578438752.gitgitgadget@gmail.com>
From: Heba Waly <heba.waly@gmail.com>
Use the advise function in advice.c to display hints to the users, as
it provides a neat and a standard format for hint messages, i.e: the
text is colored in yellow and the line starts by the word "hint:".
Also this will enable us to control the messages using advice.*
configuration variables.
Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
advice.c | 2 ++
advice.h | 1 +
builtin/add.c | 6 ++++--
t/t3700-add.sh | 2 +-
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/advice.c b/advice.c
index 249c60dcf3..098ac0abea 100644
--- a/advice.c
+++ b/advice.c
@@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;
int advice_checkout_ambiguous_remote_branch_name = 1;
int advice_nested_tag = 1;
int advice_submodule_alternate_error_strategy_die = 1;
+int advice_add_nothing = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -91,6 +92,7 @@ static struct {
{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
{ "nestedTag", &advice_nested_tag },
{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
+ { "addNothing", &advice_add_nothing },
/* make this an alias for backward compatibility */
{ "pushNonFastForward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index b706780614..83287b0594 100644
--- a/advice.h
+++ b/advice.h
@@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;
extern int advice_checkout_ambiguous_remote_branch_name;
extern int advice_nested_tag;
extern int advice_submodule_alternate_error_strategy_die;
+extern int advice_add_nothing;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/add.c b/builtin/add.c
index 4c38aff419..57b3186f69 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -390,7 +390,8 @@ static int add_files(struct dir_struct *dir, int flags)
fprintf(stderr, _(ignore_error));
for (i = 0; i < dir->ignored_nr; i++)
fprintf(stderr, "%s\n", dir->ignored[i]->name);
- fprintf(stderr, _("Use -f if you really want to add them.\n"));
+ if (advice_add_nothing)
+ advise(_("Use -f if you really want to add them.\n"));
exit_status = 1;
}
@@ -480,7 +481,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (require_pathspec && pathspec.nr == 0) {
fprintf(stderr, _("Nothing specified, nothing added.\n"));
- fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+ if (advice_add_nothing)
+ advise( _("Maybe you wanted to say 'git add .'?\n"));
return 0;
}
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index c325167b90..a649805369 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
cat >expect.err <<\EOF
The following paths are ignored by one of your .gitignore files:
ignored-file
-Use -f if you really want to add them.
+hint: Use -f if you really want to add them.
EOF
cat >expect.out <<\EOF
add 'track-this'
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/1] [Outreachy] add: use advise function to display hints
From: Heba Waly via GitGitGadget @ 2020-01-07 23:12 UTC (permalink / raw)
To: git; +Cc: Heba Waly, Junio C Hamano
In-Reply-To: <pull.508.git.1577934241.gitgitgadget@gmail.com>
The advise function in advice.c provides a neat and a standard format for
hint messages, i.e: the text is colored in yellow and the line starts by the
word "hint:". Also this will allow us to control the hint messages based on
advice.* configuration variables.
This patch suggests using this advise function whenever displaying hints to
improve the user experience, as the user's eyes will get used to the format
and will scan the screen for the yellow hints whenever confused instead of
reading all the output lines looking for advice.
Heba Waly (1):
add: use advise function to display hints
advice.c | 2 ++
advice.h | 1 +
builtin/add.c | 6 ++++--
t/t3700-add.sh | 2 +-
4 files changed, 8 insertions(+), 3 deletions(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-508%2FHebaWaly%2Fformatting_hints-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-508/HebaWaly/formatting_hints-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/508
Range-diff vs v1:
1: 90608636bf ! 1: 9f9febd3f4 add: use advise function to display hints
@@ -6,8 +6,43 @@
it provides a neat and a standard format for hint messages, i.e: the
text is colored in yellow and the line starts by the word "hint:".
+ Also this will enable us to control the messages using advice.*
+ configuration variables.
+
Signed-off-by: Heba Waly <heba.waly@gmail.com>
+ diff --git a/advice.c b/advice.c
+ --- a/advice.c
+ +++ b/advice.c
+@@
+ int advice_checkout_ambiguous_remote_branch_name = 1;
+ int advice_nested_tag = 1;
+ int advice_submodule_alternate_error_strategy_die = 1;
++int advice_add_nothing = 1;
+
+ static int advice_use_color = -1;
+ static char advice_colors[][COLOR_MAXLEN] = {
+@@
+ { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
+ { "nestedTag", &advice_nested_tag },
+ { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
++ { "addNothing", &advice_add_nothing },
+
+ /* make this an alias for backward compatibility */
+ { "pushNonFastForward", &advice_push_update_rejected }
+
+ diff --git a/advice.h b/advice.h
+ --- a/advice.h
+ +++ b/advice.h
+@@
+ extern int advice_checkout_ambiguous_remote_branch_name;
+ extern int advice_nested_tag;
+ extern int advice_submodule_alternate_error_strategy_die;
++extern int advice_add_nothing;
+
+ int git_default_advice_config(const char *var, const char *value);
+ __attribute__((format (printf, 1, 2)))
+
diff --git a/builtin/add.c b/builtin/add.c
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -16,7 +51,8 @@
for (i = 0; i < dir->ignored_nr; i++)
fprintf(stderr, "%s\n", dir->ignored[i]->name);
- fprintf(stderr, _("Use -f if you really want to add them.\n"));
-+ advise(_("Use -f if you really want to add them.\n"));
++ if (advice_add_nothing)
++ advise(_("Use -f if you really want to add them.\n"));
exit_status = 1;
}
@@ -25,7 +61,8 @@
if (require_pathspec && pathspec.nr == 0) {
fprintf(stderr, _("Nothing specified, nothing added.\n"));
- fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
-+ advise( _("Maybe you wanted to say 'git add .'?\n"));
++ if (advice_add_nothing)
++ advise( _("Maybe you wanted to say 'git add .'?\n"));
return 0;
}
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 1/9] built-in add -p: support interactive.diffFilter
From: SZEDER Gábor @ 2020-01-07 22:57 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Johannes Schindelin, Junio C Hamano
In-Reply-To: <f45ff08bd0a0a2e2aba9ae929b6e5ecb3bdd4e07.1577275020.git.gitgitgadget@gmail.com>
On Wed, Dec 25, 2019 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> The Perl version supports post-processing the colored diff (that is
> generated in addition to the uncolored diff, intended to offer a
> prettier user experience) by a command configured via that config
> setting, and now the built-in version does that, too.
So this patch makes the test 'detect bogus diffFilter output' in
't3701-add-interactive.sh' succeed with the builtin interactive add,
but I stumbled upon a test failure caused by SIGPIPE in an
experimental Travis CI s390x build:
expecting success of 3701.49 'detect bogus diffFilter output':
git reset --hard &&
echo content >test &&
test_config interactive.diffFilter "echo too-short" &&
printf y >y &&
test_must_fail force_color git add -p <y
+ git reset --hard
HEAD is now at 6ee5ee5 test
+ echo content
+ test_config interactive.diffFilter echo too-short
+ printf y
+ test_must_fail force_color git add -p
test_must_fail: died by signal 13: force_color git add -p
error: last command exited with $?=1
Turns out it's a general issue, and
GIT_TEST_ADD_I_USE_BUILTIN=1 ./t3701-add-interactive.sh -r 39,49 --stress
fails within 10 seconds on my Linux box, whereas the scripted 'add -p'
managed to survive a couple hundred repetitions.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> add-interactive.c | 12 ++++++++++++
> add-interactive.h | 3 +++
> add-patch.c | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 48 insertions(+)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index a5bb14f2f4..1786ea29c4 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -52,6 +52,17 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
> diff_get_color(s->use_color, DIFF_FILE_OLD));
> init_color(r, s, "new", s->file_new_color,
> diff_get_color(s->use_color, DIFF_FILE_NEW));
> +
> + FREE_AND_NULL(s->interactive_diff_filter);
> + git_config_get_string("interactive.difffilter",
> + &s->interactive_diff_filter);
> +}
> +
> +void clear_add_i_state(struct add_i_state *s)
> +{
> + FREE_AND_NULL(s->interactive_diff_filter);
> + memset(s, 0, sizeof(*s));
> + s->use_color = -1;
> }
>
> /*
> @@ -1149,6 +1160,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
> strbuf_release(&print_file_item_data.worktree);
> strbuf_release(&header);
> prefix_item_list_clear(&commands);
> + clear_add_i_state(&s);
>
> return res;
> }
> diff --git a/add-interactive.h b/add-interactive.h
> index b2f23479c5..46c73867ad 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -15,9 +15,12 @@ struct add_i_state {
> char context_color[COLOR_MAXLEN];
> char file_old_color[COLOR_MAXLEN];
> char file_new_color[COLOR_MAXLEN];
> +
> + char *interactive_diff_filter;
> };
>
> void init_add_i_state(struct add_i_state *s, struct repository *r);
> +void clear_add_i_state(struct add_i_state *s);
>
> struct repository;
> struct pathspec;
> diff --git a/add-patch.c b/add-patch.c
> index 46c6c183d5..78bde41df0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -398,6 +398,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>
> if (want_color_fd(1, -1)) {
> struct child_process colored_cp = CHILD_PROCESS_INIT;
> + const char *diff_filter = s->s.interactive_diff_filter;
>
> setup_child_process(s, &colored_cp, NULL);
> xsnprintf((char *)args.argv[color_arg_index], 8, "--color");
> @@ -407,6 +408,24 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> argv_array_clear(&args);
> if (res)
> return error(_("could not parse colored diff"));
> +
> + if (diff_filter) {
> + struct child_process filter_cp = CHILD_PROCESS_INIT;
> +
> + setup_child_process(s, &filter_cp,
> + diff_filter, NULL);
> + filter_cp.git_cmd = 0;
> + filter_cp.use_shell = 1;
> + strbuf_reset(&s->buf);
> + if (pipe_command(&filter_cp,
> + colored->buf, colored->len,
> + &s->buf, colored->len,
> + NULL, 0) < 0)
> + return error(_("failed to run '%s'"),
> + diff_filter);
> + strbuf_swap(colored, &s->buf);
> + }
> +
> strbuf_complete_line(colored);
> colored_p = colored->buf;
> colored_pend = colored_p + colored->len;
> @@ -531,6 +550,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> colored_pend - colored_p);
> if (colored_eol)
> colored_p = colored_eol + 1;
> + else if (p != pend)
> + /* colored shorter than non-colored? */
> + goto mismatched_output;
> else
> colored_p = colored_pend;
>
> @@ -555,6 +577,15 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> */
> hunk->splittable_into++;
>
> + /* non-colored shorter than colored? */
> + if (colored_p != colored_pend) {
> +mismatched_output:
> + error(_("mismatched output from interactive.diffFilter"));
> + advise(_("Your filter must maintain a one-to-one correspondence\n"
> + "between its input and output lines."));
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -1612,6 +1643,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> parse_diff(&s, ps) < 0) {
> strbuf_release(&s.plain);
> strbuf_release(&s.colored);
> + clear_add_i_state(&s.s);
> return -1;
> }
>
> @@ -1630,5 +1662,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> strbuf_release(&s.buf);
> strbuf_release(&s.plain);
> strbuf_release(&s.colored);
> + clear_add_i_state(&s.s);
> return 0;
> }
> --
> gitgitgadget
>
^ permalink raw reply
* Re: [PATCH v2 2/2] graph: fix lack of color in horizontal lines
From: Junio C Hamano @ 2020-01-07 21:51 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, peff, brad, sunshine, James Coglan, Derrick Stolee
In-Reply-To: <11887bd38d73b48c6677ef1e8fe2b9f0c295c8a0.1578432422.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> In some cases, horizontal lines in rendered graphs can lose their
> coloring. This is due to a use of graph_line_addch() instead of
> graph_line_write_column(). Using a ternary operator to pick the
> character is nice for compact code, but we actually need a column to
> provide the color.
>
> Add a test to t4215-log-skewed-merges.sh to prevent regression.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
Thanks. Looks good.
^ permalink raw reply
* Re: [PATCH v2 1/2] graph: fix case that hit assert()
From: Junio C Hamano @ 2020-01-07 21:50 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, peff, brad, sunshine, James Coglan, Derrick Stolee
In-Reply-To: <c05fe2c37a87b254157eec1c8a0f8ca206e1cd31.1578432422.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <dstolee@microsoft.com>
> Subject: Re: [PATCH v2 1/2] graph: fix case that hit assert()
>
> A failure was reported in "git log --graph --all" with the new
> graph-rendering logic. The code fails an assert() statement when
> collapsing multiple edges from a merge.
>
> The assert was introduced by eaf158f8 (graph API: Use horizontal
> lines for more compact graphs, 2009-04-21), which is quite old.
> This assert is trying to say that when we complete a horizontal
> line with a single slash, it is because we have reached our target.
>
> This assertion is hit when we have two collapsing lines from the
> same merge commit, as follows:
>
> | | | | *
> | |_|_|/|
> |/| | |/
> | | |/|
> | |/| |
> | * | |
> * | | |
I was sort-of expecting to see
graph: drop assert() for merge with two collapsing parents
When "git log --graph" shows a merge commit that has two collapsing
lines, like:
| | | | *
| |_|_|/|
|/| | |/
| | |/|
| |/| |
| * | |
* | | |
we trigger an assert():
graph.c:1228: graph_output_collapsing_line: Assertion
`graph->mapping[i - 3] == target' failed.
The assert was introduced by eaf158f8 ("graph API: Use horizontal
lines for more compact graphs", 2009-04-21), which is quite old.
...
near the beginning of this commit, though.
> In a larger example, the current code will output a collapse
> as follows:
>
> | | | | | | *
> | |_|_|_|_|/|\
> |/| | | | |/ /
> | | | | |/| /
> | | | |/| |/
> | | |/| |/|
> | |/| |/| |
> | | |/| | |
> | | * | | |
>
> However, the intended collapse should allow multiple horizontal lines
> as follows:
>
> | | | | | | *
> | |_|_|_|_|/|\
> |/| | | | |/ /
> | | |_|_|/| /
> | |/| | | |/
> | | | |_|/|
> | | |/| | |
> | | * | | |
>
> This behavior is not corrected by this change, but is noted for a later
> update.
This part is new and looks like a good thing to mention.
^ permalink raw reply
* [PATCH v2 2/2] graph: fix lack of color in horizontal lines
From: Derrick Stolee via GitGitGadget @ 2020-01-07 21:27 UTC (permalink / raw)
To: git
Cc: peff, brad, sunshine, James Coglan, Derrick Stolee,
Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.517.v2.git.1578432422.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
In some cases, horizontal lines in rendered graphs can lose their
coloring. This is due to a use of graph_line_addch() instead of
graph_line_write_column(). Using a ternary operator to pick the
character is nice for compact code, but we actually need a column to
provide the color.
Add a test to t4215-log-skewed-merges.sh to prevent regression.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
graph.c | 13 +++++++++----
t/t4215-log-skewed-merges.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/graph.c b/graph.c
index f514ed3efa..aaf97069bd 100644
--- a/graph.c
+++ b/graph.c
@@ -1063,7 +1063,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l
int i, j;
struct commit_list *first_parent = first_interesting_parent(graph);
- int seen_parent = 0;
+ struct column *parent_col = NULL;
/*
* Output the post-merge row
@@ -1117,12 +1117,17 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l
graph_line_addch(line, ' ');
} else {
graph_line_write_column(line, col, '|');
- if (graph->merge_layout != 0 || i != graph->commit_index - 1)
- graph_line_addch(line, seen_parent ? '_' : ' ');
+ if (graph->merge_layout != 0 || i != graph->commit_index - 1) {
+ if (parent_col)
+ graph_line_write_column(
+ line, parent_col, '_');
+ else
+ graph_line_addch(line, ' ');
+ }
}
if (col_commit == first_parent->item)
- seen_parent = 1;
+ parent_col = col;
}
/*
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index ddf6f6f5d3..5661ed5881 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -282,4 +282,33 @@ test_expect_success 'log --graph with multiple tips' '
EOF
'
+test_expect_success 'log --graph with multiple tips and colors' '
+ test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
+ cat >expect.colors <<-\EOF &&
+ * 6_I
+ <RED>|<RESET><GREEN>\<RESET>
+ <RED>|<RESET> <GREEN>|<RESET> * 6_H
+ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET><BLUE>\<RESET>
+ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 6_G
+ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 6_F
+ <RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET><GREEN>|<RESET>
+ <RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET><GREEN>/<RESET>
+ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET><GREEN>/<RESET><BLUE>|<RESET>
+ <RED>|<RESET> <GREEN>|<RESET><GREEN>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET>
+ <RED>|<RESET> <GREEN>|<RESET> * <BLUE>|<RESET> 6_E
+ <RED>|<RESET> * <CYAN>|<RESET> <BLUE>|<RESET> 6_D
+ <RED>|<RESET> <BLUE>|<RESET> <CYAN>|<RESET><BLUE>/<RESET>
+ <RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET><CYAN>|<RESET>
+ * <BLUE>|<RESET> <CYAN>|<RESET> 6_C
+ <CYAN>|<RESET> <BLUE>|<RESET><CYAN>/<RESET>
+ <CYAN>|<RESET><CYAN>/<RESET><BLUE>|<RESET>
+ * <BLUE>|<RESET> 6_B
+ <BLUE>|<RESET><BLUE>/<RESET>
+ * 6_A
+ EOF
+ git log --color=always --graph --date-order --pretty=tformat:%s 6_1 6_3 6_5 >actual.colors.raw &&
+ test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
+ test_cmp expect.colors actual.colors
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] Fix two bugs in graph.c
From: Derrick Stolee via GitGitGadget @ 2020-01-07 21:27 UTC (permalink / raw)
To: git; +Cc: peff, brad, sunshine, James Coglan, Derrick Stolee,
Junio C Hamano
In-Reply-To: <pull.517.git.1578408947.gitgitgadget@gmail.com>
This is a possible fix for the bug reported in [1].
The first commit fixes the runtime failure due to the assert() statement.
The second commit replaces the assert() statements with a macro that
triggers a BUG().
The third commit adds another test that shows a more complicated example and
how the new code in v2.25.0-rc1 has a behavior change that is not
necessarily wanted.
Thanks, -Stolee
[1]
https://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/
Derrick Stolee (2):
graph: fix case that hit assert()
graph: fix lack of color in horizontal lines
graph.c | 17 +++++----
t/t4215-log-skewed-merges.sh | 71 ++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+), 8 deletions(-)
base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-517%2Fderrickstolee%2Fgraph-assert-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-517/derrickstolee/graph-assert-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/517
Range-diff vs v1:
1: 65186f3ded ! 1: c05fe2c37a graph: fix case that hit assert()
@@ -3,17 +3,11 @@
graph: fix case that hit assert()
A failure was reported in "git log --graph --all" with the new
- graph-rendering logic. Create a test case that matches the
- topology of that example and uses an explicit ref ordering instead
- of the "--all" option. The test would fail with the following error:
+ graph-rendering logic. The code fails an assert() statement when
+ collapsing multiple edges from a merge.
- graph.c:1228: graph_output_collapsing_line: Assertion
- `graph->mapping[i - 3] == target' failed.
-
- The situation is a little complicated, so let's break it down.
-
- The assert was introduced by eaf158f8 ("graph API: Use horizontal
- lines for more compact graphs", 2009-04-21), which is quite old.
+ The assert was introduced by eaf158f8 (graph API: Use horizontal
+ lines for more compact graphs, 2009-04-21), which is quite old.
This assert is trying to say that when we complete a horizontal
line with a single slash, it is because we have reached our target.
@@ -30,18 +24,45 @@
It is actually the _second_ collapsing line that hits this assert.
The reason we are in this code path is because we are collapsing
- the first line, and it in that case we are hitting our target now
+ the first line, and in that case we are hitting our target now
that the horizontal line is complete. However, the second line
cannot be a horizontal line, so it will collapse without horizontal
lines. In this case, it is inappropriate to assert that we have
reached our target, as we need to continue for another column
before reaching the target. Dropping the assert is safe here.
- Second, the horizontal lines in that first line drop their coloring.
- This is due to a use of graph_line_addch() instead of
- graph_line_write_column(). Using a ternary operator to pick the
- character is nice for compact code, but we actually need a column
- to provide the color.
+ The new behavior in 0f0f389f12 (graph: tidy up display of
+ left-skewed merges, 2019-10-15) caused the behavior change that
+ made this assertion failure possible. In addition to making the
+ assert possible, it also changed how multiple edges collapse.
+
+ In a larger example, the current code will output a collapse
+ as follows:
+
+ | | | | | | *
+ | |_|_|_|_|/|\
+ |/| | | | |/ /
+ | | | | |/| /
+ | | | |/| |/
+ | | |/| |/|
+ | |/| |/| |
+ | | |/| | |
+ | | * | | |
+
+ However, the intended collapse should allow multiple horizontal lines
+ as follows:
+
+ | | | | | | *
+ | |_|_|_|_|/|\
+ |/| | | | |/ /
+ | | |_|_|/| /
+ | |/| | | |/
+ | | | |_|/|
+ | | |/| | |
+ | | * | | |
+
+ This behavior is not corrected by this change, but is noted for a later
+ update.
Helped-by: Jeff King <peff@peff.net>
Reported-by: Bradley Smith <brad@brad-smith.co.uk>
@@ -50,36 +71,6 @@
diff --git a/graph.c b/graph.c
--- a/graph.c
+++ b/graph.c
-@@
- int i, j;
-
- struct commit_list *first_parent = first_interesting_parent(graph);
-- int seen_parent = 0;
-+ struct column *parent_col = NULL;
-
- /*
- * Output the post-merge row
-@@
- graph_line_addch(line, ' ');
- } else {
- graph_line_write_column(line, col, '|');
-- if (graph->merge_layout != 0 || i != graph->commit_index - 1)
-- graph_line_addch(line, seen_parent ? '_' : ' ');
-+ if (graph->merge_layout != 0 || i != graph->commit_index - 1) {
-+ if (parent_col)
-+ graph_line_write_column(
-+ line, parent_col, '_');
-+ else
-+ graph_line_addch(line, ' ');
-+ }
- }
-
- if (col_commit == first_parent->item)
-- seen_parent = 1;
-+ parent_col = col;
- }
-
- /*
@@
*
* The space just to the left of this
2: 5dd305d2f0 ! 2: 11887bd38d graph: replace assert() with graph_assert() macro
@@ -1,100 +1,86 @@
Author: Derrick Stolee <dstolee@microsoft.com>
- graph: replace assert() with graph_assert() macro
+ graph: fix lack of color in horizontal lines
- The assert() macro is sometimes compiled out. Instead, switch these into
- BUG() statements using our own custom macro.
+ In some cases, horizontal lines in rendered graphs can lose their
+ coloring. This is due to a use of graph_line_addch() instead of
+ graph_line_write_column(). Using a ternary operator to pick the
+ character is nice for compact code, but we actually need a column to
+ provide the color.
- Reported-by: Eric Sunshine <sunshine@sunshineco.com>
+ Add a test to t4215-log-skewed-merges.sh to prevent regression.
+
+ Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
diff --git a/graph.c b/graph.c
--- a/graph.c
+++ b/graph.c
@@
- #include "revision.h"
- #include "argv-array.h"
-
-+#define graph_assert(exp) if (!(exp)) { BUG("assert failed: "#exp""); }
-+
- /* Internal API */
-
- /*
-@@
- struct git_graph *graph = data;
- static struct strbuf msgbuf = STRBUF_INIT;
-
-- assert(opt);
-+ graph_assert(opt);
-
- strbuf_reset(&msgbuf);
- if (opt->line_prefix)
-@@
- *
- * We need 2 extra rows for every parent over 2.
- */
-- assert(graph->num_parents >= 3);
-+ graph_assert(graph->num_parents >= 3);
+ int i, j;
- /*
- * graph->expansion_row tracks the current expansion row we are on.
- * It should be in the range [0, num_expansion_rows - 1]
- */
-- assert(0 <= graph->expansion_row &&
-+ graph_assert(0 <= graph->expansion_row &&
- graph->expansion_row < graph_num_expansion_rows(graph));
+ struct commit_list *first_parent = first_interesting_parent(graph);
+- int seen_parent = 0;
++ struct column *parent_col = NULL;
/*
+ * Output the post-merge row
@@
- * (We should only see boundary commits when revs->boundary is set.)
- */
- if (graph->commit->object.flags & BOUNDARY) {
-- assert(graph->revs->boundary);
-+ graph_assert(graph->revs->boundary);
- graph_line_addch(line, 'o');
- return;
+ graph_line_addch(line, ' ');
+ } else {
+ graph_line_write_column(line, col, '|');
+- if (graph->merge_layout != 0 || i != graph->commit_index - 1)
+- graph_line_addch(line, seen_parent ? '_' : ' ');
++ if (graph->merge_layout != 0 || i != graph->commit_index - 1) {
++ if (parent_col)
++ graph_line_write_column(
++ line, parent_col, '_');
++ else
++ graph_line_addch(line, ' ');
++ }
+ }
+
+ if (col_commit == first_parent->item)
+- seen_parent = 1;
++ parent_col = col;
}
/*
-- * get_revision_mark() handles all other cases without assert()
-+ * get_revision_mark() handles all other cases without graph_assert()
- */
- graph_line_addstr(line, get_revision_mark(graph->revs, graph->commit));
- }
-@@
-
- for (j = 0; j < graph->num_parents; j++) {
- par_column = graph_find_new_column_by_commit(graph, parents->item);
-- assert(par_column >= 0);
-+ graph_assert(par_column >= 0);
-
- c = merge_chars[idx];
- graph_line_write_column(line, &graph->new_columns[par_column], c);
+
+ diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
+ --- a/t/t4215-log-skewed-merges.sh
+ +++ b/t/t4215-log-skewed-merges.sh
@@
- * the graph much more legible, since whenever branches
- * cross, only one is moving directions.
- */
-- assert(target * 2 <= i);
-+ graph_assert(target * 2 <= i);
+ EOF
+ '
- if (target * 2 == i) {
- /*
- * This column is already in the
- * correct place
- */
-- assert(graph->mapping[i] == -1);
-+ graph_assert(graph->mapping[i] == -1);
- graph->mapping[i] = target;
- } else if (graph->mapping[i - 1] < 0) {
- /*
-@@
- * The space just to the left of this
- * branch should always be empty.
- */
-- assert(graph->mapping[i - 1] > target);
-- assert(graph->mapping[i - 2] < 0);
-+ graph_assert(graph->mapping[i - 1] > target);
-+ graph_assert(graph->mapping[i - 2] < 0);
- graph->mapping[i - 2] = target;
- /*
- * Mark this branch as the horizontal edge to
++test_expect_success 'log --graph with multiple tips and colors' '
++ test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
++ cat >expect.colors <<-\EOF &&
++ * 6_I
++ <RED>|<RESET><GREEN>\<RESET>
++ <RED>|<RESET> <GREEN>|<RESET> * 6_H
++ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET><BLUE>\<RESET>
++ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 6_G
++ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 6_F
++ <RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET><GREEN>|<RESET>
++ <RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET><GREEN>/<RESET>
++ <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET><GREEN>/<RESET><BLUE>|<RESET>
++ <RED>|<RESET> <GREEN>|<RESET><GREEN>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET>
++ <RED>|<RESET> <GREEN>|<RESET> * <BLUE>|<RESET> 6_E
++ <RED>|<RESET> * <CYAN>|<RESET> <BLUE>|<RESET> 6_D
++ <RED>|<RESET> <BLUE>|<RESET> <CYAN>|<RESET><BLUE>/<RESET>
++ <RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET><CYAN>|<RESET>
++ * <BLUE>|<RESET> <CYAN>|<RESET> 6_C
++ <CYAN>|<RESET> <BLUE>|<RESET><CYAN>/<RESET>
++ <CYAN>|<RESET><CYAN>/<RESET><BLUE>|<RESET>
++ * <BLUE>|<RESET> 6_B
++ <BLUE>|<RESET><BLUE>/<RESET>
++ * 6_A
++ EOF
++ git log --color=always --graph --date-order --pretty=tformat:%s 6_1 6_3 6_5 >actual.colors.raw &&
++ test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
++ test_cmp expect.colors actual.colors
++'
++
+ test_done
3: f74e82bea6 < -: ---------- t4215: add bigger graph collapse test
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/2] graph: fix case that hit assert()
From: Derrick Stolee via GitGitGadget @ 2020-01-07 21:27 UTC (permalink / raw)
To: git
Cc: peff, brad, sunshine, James Coglan, Derrick Stolee,
Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.517.v2.git.1578432422.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
A failure was reported in "git log --graph --all" with the new
graph-rendering logic. The code fails an assert() statement when
collapsing multiple edges from a merge.
The assert was introduced by eaf158f8 (graph API: Use horizontal
lines for more compact graphs, 2009-04-21), which is quite old.
This assert is trying to say that when we complete a horizontal
line with a single slash, it is because we have reached our target.
This assertion is hit when we have two collapsing lines from the
same merge commit, as follows:
| | | | *
| |_|_|/|
|/| | |/
| | |/|
| |/| |
| * | |
* | | |
It is actually the _second_ collapsing line that hits this assert.
The reason we are in this code path is because we are collapsing
the first line, and in that case we are hitting our target now
that the horizontal line is complete. However, the second line
cannot be a horizontal line, so it will collapse without horizontal
lines. In this case, it is inappropriate to assert that we have
reached our target, as we need to continue for another column
before reaching the target. Dropping the assert is safe here.
The new behavior in 0f0f389f12 (graph: tidy up display of
left-skewed merges, 2019-10-15) caused the behavior change that
made this assertion failure possible. In addition to making the
assert possible, it also changed how multiple edges collapse.
In a larger example, the current code will output a collapse
as follows:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | | | |/| /
| | | |/| |/
| | |/| |/|
| |/| |/| |
| | |/| | |
| | * | | |
However, the intended collapse should allow multiple horizontal lines
as follows:
| | | | | | *
| |_|_|_|_|/|\
|/| | | | |/ /
| | |_|_|/| /
| |/| | | |/
| | | |_|/|
| | |/| | |
| | * | | |
This behavior is not corrected by this change, but is noted for a later
update.
Helped-by: Jeff King <peff@peff.net>
Reported-by: Bradley Smith <brad@brad-smith.co.uk>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
graph.c | 4 ----
t/t4215-log-skewed-merges.sh | 42 ++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/graph.c b/graph.c
index 66ae18add8..f514ed3efa 100644
--- a/graph.c
+++ b/graph.c
@@ -1219,13 +1219,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l
*
* The space just to the left of this
* branch should always be empty.
- *
- * The branch to the left of that space
- * should be our eventual target.
*/
assert(graph->mapping[i - 1] > target);
assert(graph->mapping[i - 2] < 0);
- assert(graph->mapping[i - 3] == target);
graph->mapping[i - 2] = target;
/*
* Mark this branch as the horizontal edge to
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 18709a723e..ddf6f6f5d3 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
EOF
'
+test_expect_success 'log --graph with multiple tips' '
+ git checkout --orphan 6_1 &&
+ test_commit 6_A &&
+ git branch 6_2 &&
+ git branch 6_4 &&
+ test_commit 6_B &&
+ git branch 6_3 &&
+ test_commit 6_C &&
+ git checkout 6_2 && test_commit 6_D &&
+ git checkout 6_3 && test_commit 6_E &&
+ git checkout -b 6_5 6_1 &&
+ git merge --no-ff 6_2 -m 6_F &&
+ git checkout 6_4 && test_commit 6_G &&
+ git checkout 6_3 &&
+ git merge --no-ff 6_4 -m 6_H &&
+ git checkout 6_1 &&
+ git merge --no-ff 6_2 -m 6_I &&
+
+ check_graph 6_1 6_3 6_5 <<-\EOF
+ * 6_I
+ |\
+ | | * 6_H
+ | | |\
+ | | | * 6_G
+ | | * | 6_E
+ | | | | * 6_F
+ | |_|_|/|
+ |/| | |/
+ | | |/|
+ | |/| |
+ | * | | 6_D
+ | | |/
+ | |/|
+ * | | 6_C
+ | |/
+ |/|
+ * | 6_B
+ |/
+ * 6_A
+ EOF
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 0/3] t: rework tests for --pathspec-from-file
From: Junio C Hamano @ 2020-01-07 21:13 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget
Cc: git, Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v4.git.1577787313.gitgitgadget@gmail.com>
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This branch is a follow-up for [1] where part of branch was merged into `master` via [2].
>
> Previously in [3] there were some concerns on whether removing
> copy&pasted tests is good. I still think that yes, it 's a good thing,
> mostly because of high volume of potential 13*6=78 duplicate tests.
>
> Still, I separated this change as last patch, so that the remaining
> part of the branch can be taken without it.
With the third step the series won't merge cleanly with other topic
you have in 'next' (t7107 gets somewhat heavy merge conflicts).
I'll queue the first two for now but let's clean them up post 2.25
release.
Thanks.
^ permalink raw reply
* Re: SHA-1 chosen-prefix colission attack
From: Santiago Torres Arias @ 2020-01-07 20:31 UTC (permalink / raw)
To: Kevin Daudt, git
In-Reply-To: <20200107173111.GB923852@alpha>
[-- Attachment #1: Type: text/plain, Size: 425 bytes --]
> > 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,
Cheers
-Santiago
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy] Return value before or after free()?
From: Miriam R. @ 2020-01-07 20:40 UTC (permalink / raw)
To: brian m. carlson, Jeff King, Miriam R., git
In-Reply-To: <20200107015859.GJ6570@camp.crustytoothpaste.net>
El mar., 7 ene. 2020 a las 2:59, brian m. carlson
(<sandals@crustytoothpaste.net>) escribió:
>
> On 2020-01-07 at 01:08:09, brian m. carlson wrote:
> > Unfortunately, compilers have gotten much more aggressive about assuming
> > that undefined behavior never occurs and rewriting code based on that.
> > clang is not as bad about doing that, but GCC is very aggressive about
> > it. There are multiple instances where NULL pointer checks have been
> > optimized out because the compiler exploited undefined behavior to
> > assume a pointer was never NULL.
> >
> > In this case, the only case in which we can safely assume that this
> > behavior is acceptable is that r is NULL, in which case C11 tells us
> > that "no action occurs" due to the free. So the compiler could just
> > optimize this out to a "return 0". Just because it doesn't now doesn't
> > mean we can assume it won't in the future, so we do need to fix this.
> >
> > I'll send a patch.
>
> Oof, I just realized that you had tagged this with "[Outreachy]", which
> means that you were probably planning on sending a patch to fix this,
> and then I went and did it instead, so let me apologize for doing that.
>
Sorry for my late reply, but I have been traveling all day.
Don't worry Brian, I am working on finishing bisect-helper conversion
from shell to C.
I am planning to send a patch related with this function as part of a
patch series. My patch only changes the static header to be used in
bisect related files.
My mentor (Christian Couder) detected this and suggested me to ask the
community.
> I sent it because oftentimes we say "we should fix this thing" and then
> never do it because nobody sends a patch, but in this case I should have
> paid more attention and waited for you to respond and send one instead.
>
Don't worry again, next time if my question is related to a patch I am
going to send I will actively write it and this way there will be no
confusion. :)
> Again, sorry about that.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204
^ permalink raw reply
* Re: [PATCH 1/3] graph: fix case that hit assert()
From: Junio C Hamano @ 2020-01-07 20:21 UTC (permalink / raw)
To: Jeff King
Cc: Derrick Stolee via GitGitGadget, git, brad, sunshine,
Derrick Stolee
In-Reply-To: <20200107193143.GA56858@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> >> Second, the horizontal lines in that first line drop their coloring.
>> >> This is due to a use of graph_line_addch() instead of
>> >> graph_line_write_column(). Using a ternary operator to pick the
>> >> character is nice for compact code, but we actually need a column
>> >> to provide the color.
>> >
>> > It seems like this is a totally separate bug, and could be its own
>> > commit?
>>
>> I think so.
>>
>> And with that removed, all that remains would be a removal of the
>> assert() plus an additional test?
>
> Yes, though note that the color thing is a v2.25 regression as well. So
> we'd probably want both of them.
Sure. Those two would make perfect pair of commits to finish -rc2 with.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Junio C Hamano @ 2020-01-07 20:19 UTC (permalink / raw)
To: Liam Huang via GitGitGadget; +Cc: git
In-Reply-To: <8c96c56a1818b066d4570873e00c52d42399e3c2.1578391376.git.gitgitgadget@gmail.com>
"Liam Huang via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Liam Huang <Liam0205@users.noreply.github.com>
>
> Some APIs have been changed since OpenSSL 1.1.0, so fix incompatibilities with OpenSSL 1.1.x.
I wonder if the patch can be made a lot less noisy with something
along this line
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#define OPENSSL_sk_num(x) sk_GENERAL_NAME_num(x)
#define OPENSSL_sk_value(x,y) sk_GENERAL_NAME_value((x),(y))
#define OPENSSL_sk_pop_free(x,y) sk_GENERAL_NAME_pop_free((x),(y))
#endif
which would allow you to reduce many #if/#else/#endif in the actual
code.
^ permalink raw reply
* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Junio C Hamano @ 2020-01-07 20:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <xmqq7e2359an.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
>> Of course, I can teach GitGitGadget to not Cc: you. Like, always. Not sure
>> that you would like that any better because you would not even be Cc:ed
>> once consensus was reached.
>
> That would actually be better. Somebody in the discussion thread
s/better/much much &/;
> would probably say "This is good enough---send it to the maintainer"
> when the topic is ready.
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. And
by treating the maintainer merely just one of the reviewer, that
mechanism can naturally be reused.
^ permalink raw reply
* Re: [PATCH v3 10/15] rebase: add an --am option
From: Junio C Hamano @ 2020-01-07 20:11 UTC (permalink / raw)
To: Elijah Newren
Cc: Phillip Wood, Elijah Newren via GitGitGadget, Git Mailing List,
Johannes Schindelin, Denton Liu, Pavel Roskin, Alban Gruin,
SZEDER Gábor
In-Reply-To: <CABPp-BEoTb6LVXThEM4zoKxVOnzBNs7y-Mk+oFbb6BUzCo3RHg@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> However, I thought of this option before Junio suggested a
> rebase.backend config setting, so we could just rely on that instead.
> Thus, getting rid of the "--am" flag in detail would mean:
> * I need to redo the test changes in this series to use "-c
> rebase.backend=am" instead of "--am"
> * It will be slightly harder for users to use the escape hatch in
> one-off cases during the transition
> * We need to figure out the right way to reword the documentation
>
> The first two are pretty minor, so that probably means I just need to
> come up with some good wording for the documentation (suggestions
> welcome...)
It probably is a good idea to keep --am (and possibly --[no-]am) as
long as rebase.backend option exists. A configuration variable that
changes behaviour without allowing it to be overridden from the
command line is not a good idea.
^ permalink raw reply
* Re: [PATCH 1/1] doc: fix a typo in gitcore-tutorial.txt
From: Junio C Hamano @ 2020-01-07 19:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Heba Waly
In-Reply-To: <CAPig+cTv5SOxEjjVQ7QvqJ1WbZGbcXegcCP4d5CK+nSdJvkNdQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> To save reviewers the time and effort of having to figure all this
> out, use the commit message to explain the situation. For example, you
> might say:
>
> doc/gitcore-tutorial: fix prose to match example command
>
> In 328c6cb853 (doc: promote "git switch", 2019-03-29), an example
> was changed to use "git switch" rather than "git checkout" but an
> instance of "git checkout" in the explanatory text preceding the
> example was overlooked. Fix this oversight.
Thanks for a ncie educational session ;-)
^ permalink raw reply
* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Junio C Hamano @ 2020-01-07 19:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <nycvar.QRO.7.76.6.2001071944250.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I am afraid that I do not know of any means to teach GitGitGadget to make
> that call whether it has seen a consensus.
>
> And I fear that you are asking me to punt back that decision to
> contributors, i.e. put a lot of the burden of knowing how Git
> contributions are expected to progress _away_ from GitGitGadget.
Yes, and that is why I am giving review comments to contributors to
teach how the development community works.
> Of course, I can teach GitGitGadget to not Cc: you. Like, always. Not sure
> that you would like that any better because you would not even be Cc:ed
> once consensus was reached.
That would actually be better. Somebody in the discussion thread
would probably say "This is good enough---send it to the maintainer"
when the topic is ready.
^ 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