* Re: [PATCH v4] unit tests: Add a project plan document
From: Junio C Hamano @ 2023-07-01 0:42 UTC (permalink / raw)
To: Josh Steadmon
Cc: git, szeder.dev, phillip.wood123, chooglen, avarab, sandals,
calvinwan
In-Reply-To: <0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
I'll normalize this one to match prevailing use.
> Coauthored-by: Calvin Wan <calvinwan@google.com>
$ git log --since=6.months --pretty=raw --no-merges |
sed -n -e 's/^ \([^ :]*-by:\).*/\1/p' |
sort | uniq -c | sort -n | sed -e '/^ *1 /d'
5 Tested-by:
15 Suggested-by:
24 Co-authored-by:
30 Reported-by:
34 Reviewed-by:
38 Helped-by:
68 Acked-by:
1786 Signed-off-by:
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
Thanks.
^ permalink raw reply
* Re: [PATCH v4] unit tests: Add a project plan document
From: Junio C Hamano @ 2023-07-01 1:03 UTC (permalink / raw)
To: Josh Steadmon
Cc: git, szeder.dev, phillip.wood123, chooglen, avarab, sandals,
calvinwan
In-Reply-To: <0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions). Describe what we hope to accomplish by
> implementing unit tests, and explain some open questions and milestones.
> Discuss desired features for test frameworks/harnesses, and provide a
> preliminary comparison of several different frameworks.
>
> Coauthored-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> TODOs remaining:
> - List rough priorities across comparison dimensions
> - Group dimensions into sensible categories
> - Discuss pre-existing harnesses for the current test suite
> - Discuss harness vs. framework features, particularly for parallelism
> - Figure out how to evaluate frameworks on additional OSes such as *BSD
> and NonStop
> - Add more discussion about desired features (particularly mocking)
> - Add dimension for test timing
> - Evaluate remaining missing comparison table entries
Listing these explicitly here is very much appreciated. Thanks.
^ permalink raw reply
* [PATCH v1 0/3] check-attr: integrate with sparse-index
From: Shuqi Liang @ 2023-07-01 6:48 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster
Turn on sparse-index feature within `git check-attr` command.
Add necessary modifications and test them.
Shuqi Liang (3):
attr.c: read attributes in a sparse directory
t1092: add tests for `git check-attr`
check-attr: integrate with sparse-index
attr.c | 64 ++++++++++++++++--------
builtin/check-attr.c | 3 ++
t/t1092-sparse-checkout-compatibility.sh | 40 +++++++++++++++
3 files changed, 86 insertions(+), 21 deletions(-)
base-commit: 9748a6820043d5815bee770ffa51647e0adc2cf0
--
2.39.0
^ permalink raw reply
* [PATCH v1 1/3] attr.c: read attributes in a sparse directory
From: Shuqi Liang @ 2023-07-01 6:48 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster
In-Reply-To: <20230701064843.147496-1-cheskaqiqi@gmail.com>
Before this patch,`git check-attr` can't find the attributes of a file
within a sparse directory. In order to read attributes from
'.gitattributes' files that may be in a sparse directory:
When path is in cone mode of sparse checkout:
1.If path is a sparse directory, read the tree OIDs from the sparse
directory.
2.If path is a regular files, read the attributes directly from the blob
data stored in the cache.
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
attr.c | 64 +++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 43 insertions(+), 21 deletions(-)
diff --git a/attr.c b/attr.c
index 7d39ac4a29..b0d26da102 100644
--- a/attr.c
+++ b/attr.c
@@ -808,35 +808,57 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,
static struct attr_stack *read_attr_from_index(struct index_state *istate,
const char *path, unsigned flags)
{
+ struct attr_stack *stack = NULL;
+ int i;
+ struct strbuf path1 = STRBUF_INIT;
+ struct strbuf path2 = STRBUF_INIT;
+ char *first_slash = NULL;
char *buf;
unsigned long size;
if (!istate)
return NULL;
- /*
- * The .gitattributes file only applies to files within its
- * parent directory. In the case of cone-mode sparse-checkout,
- * the .gitattributes file is sparse if and only if all paths
- * within that directory are also sparse. Thus, don't load the
- * .gitattributes file since it will not matter.
- *
- * In the case of a sparse index, it is critical that we don't go
- * looking for a .gitattributes file, as doing so would cause the
- * index to expand.
- */
- if (!path_in_cone_mode_sparse_checkout(path, istate))
- return NULL;
-
- buf = read_blob_data_from_index(istate, path, &size);
- if (!buf)
- return NULL;
- if (size >= ATTR_MAX_FILE_SIZE) {
- warning(_("ignoring overly large gitattributes blob '%s'"), path);
- return NULL;
+ first_slash = strchr(path, '/');
+ if (first_slash) {
+ strbuf_add(&path1, path, first_slash - path + 1);
+ strbuf_addstr(&path2, first_slash + 1);
}
- return read_attr_from_buf(buf, path, flags);
+ if(!path_in_cone_mode_sparse_checkout(path, istate)){
+ for (i = 0; i < istate->cache_nr; i++) {
+ struct cache_entry *ce = istate->cache[i];
+ if ( !strcmp(istate->cache[i]->name, path1.buf)&&S_ISSPARSEDIR(ce->ce_mode)) {
+ stack = read_attr_from_blob(istate, &ce->oid, path2.buf, flags);
+ }else if(S_ISREG(ce->ce_mode) && !strcmp(istate->cache[i]->name, path)){
+ unsigned long sz;
+ enum object_type type;
+ void *data;
+
+ data = repo_read_object_file(the_repository, &istate->cache[i]->oid,
+ &type, &sz);
+ if (!data || type != OBJ_BLOB) {
+ free(data);
+ strbuf_release(&path1);
+ strbuf_release(&path2);
+ return NULL;
+ }
+ stack = read_attr_from_buf(data, path, flags);
+ }
+ }
+ }else{
+ buf = read_blob_data_from_index(istate, path, &size);
+ if (!buf)
+ return NULL;
+ if (size >= ATTR_MAX_FILE_SIZE) {
+ warning(_("ignoring overly large gitattributes blob '%s'"), path);
+ return NULL;
+ }
+ stack = read_attr_from_buf(buf, path, flags);
+ }
+ strbuf_release(&path1);
+ strbuf_release(&path2);
+ return stack;
}
static struct attr_stack *read_attr(struct index_state *istate,
--
2.39.0
^ permalink raw reply related
* [PATCH v1 2/3] t1092: add tests for `git check-attr`
From: Shuqi Liang @ 2023-07-01 6:48 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster
In-Reply-To: <20230701064843.147496-1-cheskaqiqi@gmail.com>
Add tests for `git check-attr`, make sure it behaves as expected when
path is both inside or outside of sparse-checkout definition.
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
t/t1092-sparse-checkout-compatibility.sh | 29 ++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 8a95adf4b5..4edfa3c168 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2259,4 +2259,33 @@ test_expect_success 'worktree is not expanded' '
ensure_not_expanded worktree remove .worktrees/hotfix
'
+test_expect_success 'check-attr with pathspec inside sparse definition' '
+ init_repos &&
+
+ echo "a -crlf myAttr" >>.gitattributes &&
+ run_on_all cp ../.gitattributes ./deep &&
+
+ test_all_match git check-attr -a -- deep/a &&
+
+ test_all_match git add deep/.gitattributes &&
+ test_all_match git check-attr -a --cached -- deep/a
+'
+
+test_expect_success 'check-attr with pathspec outside sparse definition' '
+ init_repos &&
+
+ echo "a -crlf myAttr" >>.gitattributes &&
+ run_on_sparse mkdir folder1 &&
+ run_on_all cp ../.gitattributes ./folder1 &&
+ run_on_all cp a folder1/a &&
+
+ test_all_match git check-attr -a -- folder1/a &&
+
+ git -C full-checkout add folder1/.gitattributes &&
+ run_on_sparse git add --sparse folder1/.gitattributes &&
+ run_on_all git commit -m "add .gitattributes" &&
+ test_sparse_match git sparse-checkout reapply &&
+ test_all_match git check-attr -a --cached -- folder1/a
+'
+
test_done
--
2.39.0
^ permalink raw reply related
* [PATCH v1 3/3] check-attr: integrate with sparse-index
From: Shuqi Liang @ 2023-07-01 6:48 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster
In-Reply-To: <20230701064843.147496-1-cheskaqiqi@gmail.com>
Set the requires-full-index to false for "diff-tree".
Add test to ensure the index is not expanded when the
sparse index is enabled.
The `p2000` tests demonstrate a ~63% execution time reduction for
'git check-attr' using a sparse index.
Test before after
-----------------------------------------------------------------------
2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%
2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%
2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%
2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%
Helped-by: Victoria Dye <vdye@github.com>
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
builtin/check-attr.c | 3 +++
t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++
2 files changed, 14 insertions(+)
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index b22ff748c3..02267f9bc1 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, check_attr_options,
check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
+ prepare_repo_settings(the_repository);
+ the_repository->settings.command_requires_full_index = 0;
+
if (repo_read_index(the_repository) < 0) {
die("invalid cache");
}
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 4edfa3c168..317ccc8ec5 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2288,4 +2288,15 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '
test_all_match git check-attr -a --cached -- folder1/a
'
+test_expect_success 'sparse-index is not expanded: check-attr' '
+ init_repos &&
+
+ echo "a -crlf myAttr" >>.gitattributes &&
+ run_on_all cp ../.gitattributes ./deep &&
+
+ ensure_not_expanded check-attr -a -- deep/a &&
+ run_on_all git add deep/.gitattributes &&
+ ensure_not_expanded check-attr -a --cached -- deep/a
+'
+
test_done
--
2.39.0
^ permalink raw reply related
* [PATCH] pkt-line: don't check string length in packet_length()
From: René Scharfe @ 2023-07-01 7:05 UTC (permalink / raw)
To: Git List
hex2chr() takes care not to run over the end of a short string.
101736a14c (pkt-line: extern packet_length(), 2020-05-19) turned the
input parameter of packet_length() from a string pointer into an array
of known length, making string length checks unnecessary. Get rid of
them by using hexval() directly.
The resulting branchless code is simpler and it becomes easier to see
that the function mirrors set_packet_header().
Signed-off-by: René Scharfe <l.s.r@web.de>
---
pkt-line.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 62b4208b66..6e022029ca 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -375,8 +375,10 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
int packet_length(const char lenbuf_hex[4])
{
- int val = hex2chr(lenbuf_hex);
- return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
+ return hexval(lenbuf_hex[0]) << 12 |
+ hexval(lenbuf_hex[1]) << 8 |
+ hexval(lenbuf_hex[2]) << 4 |
+ hexval(lenbuf_hex[3]);
}
static char *find_packfile_uri_path(const char *buffer)
--
2.41.0
^ permalink raw reply related
* Re: What's cooking in git.git (Jun 2023, #08; Fri, 30)
From: Vinayak Dev @ 2023-07-01 8:14 UTC (permalink / raw)
To: Junio C Hamano, Emily Shaffer; +Cc: git
In-Reply-To: <xmqq5y747l16.fsf@gitster.g>
On Sat, 1 Jul 2023 at 02:24, Junio C Hamano <gitster@pobox.com> wrote:
>
> Here are the topics that have been cooking in my tree. Commits
> prefixed with '+' are in 'next' (being in 'next' is a sign that a
> topic is stable enough to be used and are candidate to be in a
> future release). Commits prefixed with '-' are only in 'seen', and
> aren't considered "accepted" at all and may be annotated with an URL
> to a message that raises issues but they are no means exhaustive. A
> topic without enough support may be discarded after a long period of
> no activity (of course they can be resubmit when new interests
> arise).
> * vd/adjust-mfow-doc-to-updated-headers (2023-06-29) 1 commit
> - docs: include "trace.h" in MyFirstObjectWalk.txt
>
> Code snippets in a tutorial document no longer compiled after
> recent header shuffling, which have been corrected.
>
> Will merge to 'next'?
> source: <20230629185238.58961-1-vinayakdev.sci@gmail.com>
>
I found an error in this patch while fixing Emily's branch to which the tutorial
points, which I linked in a prior mail[1]. This would also require "hex.h" to be
added down in the tutorial where the function oid_to_hex() has been called.
Accordingly, I have fixed this mistake and rebased, and will send it
as a resubmission.
That should allow the code to compile properly.
[1]: https://lore.kernel.org/git/CADE8NapQK2ouy4YDQA+3NNkUn_EegkSBQCtDfcSCVGmZvVufXg@mail.gmail.com/
Thanks a lot!
Vinayak
^ permalink raw reply
* [PATCH] MyFirstObjectWalk: include necessary header files
From: Vinayak Dev @ 2023-07-01 8:28 UTC (permalink / raw)
To: gitster; +Cc: git, Vinayak Dev
In Documentation/MyFirstObjectWalk.txt, the functions
trace_printf() and oid_to_hex() are used to enable trace
output and get object ids as strings respectively. However,
the files "trace.h" and "hex.h" are not included, which
produces an error when the code from the tutorial is compiled,
with the compiler complaining that the functions are not defined
before usage. Therefore, add includes for "trace.h" and "hex.h"
in the tutorial.
Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
---
Documentation/MyFirstObjectWalk.txt | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index eee513e86f..bb99e2bb6e 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -41,6 +41,7 @@ Open up a new file `builtin/walken.c` and set up the command handler:
*/
#include "builtin.h"
+#include "trace.h"
int cmd_walken(int argc, const char **argv, const char *prefix)
{
@@ -49,12 +50,13 @@ int cmd_walken(int argc, const char **argv, const char *prefix)
}
----
-NOTE: `trace_printf()` differs from `printf()` in that it can be turned on or
-off at runtime. For the purposes of this tutorial, we will write `walken` as
-though it is intended for use as a "plumbing" command: that is, a command which
-is used primarily in scripts, rather than interactively by humans (a "porcelain"
-command). So we will send our debug output to `trace_printf()` instead. When
-running, enable trace output by setting the environment variable `GIT_TRACE`.
+NOTE: `trace_printf()`, defined in `trace.h`, differs from `printf()` in
+that it can be turned on or off at runtime. For the purposes of this
+tutorial, we will write `walken` as though it is intended for use as
+a "plumbing" command: that is, a command which is used primarily in
+scripts, rather than interactively by humans (a "porcelain" command).
+So we will send our debug output to `trace_printf()` instead.
+When running, enable trace output by setting the environment variable `GIT_TRACE`.
Add usage text and `-h` handling, like all subcommands should consistently do
(our test suite will notice and complain if you fail to do so).
@@ -805,6 +807,10 @@ just walks of commits. First, we'll make our handlers chattier - modify
go:
----
+#include "hex.h"
+
+...
+
static void walken_show_commit(struct commit *cmt, void *buf)
{
trace_printf("commit: %s\n", oid_to_hex(&cmt->object.oid));
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
--
2.41.0
^ permalink raw reply related
* [PATCH v2] t0091-bugreport.sh: actually verify some content of report
From: Martin Ågren @ 2023-07-01 19:26 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Ævar Arnfjörð Bjarmason,
Junio C Hamano, SZEDER Gábor
In-Reply-To: <YHYZTLl90rkWWVOr@google.com>
In the first test in this script, 'creates a report with content in the
right places', we generate a report and pipe it into our helper
`check_all_headers_populated()`. The idea of the helper is to find all
lines that look like headers ("[Some Header Here]") and to check that
the next line is non-empty. This is supposed to catch erroneous outputs
such as the following:
[A Header]
something
more here
[Another Header]
[Too Early Header]
contents
However, we provide the lines of the bug report as filenames to grep,
meaning we mostly end up spewing errors:
grep: : No such file or directory
grep: [System Info]: No such file or directory
grep: git version:: No such file or directory
grep: git version 2.41.0.2.gfb7d80edca: No such file or directory
This doesn't disturb the test, which tugs along and reports success, not
really having verified the contents of the report at all.
Note that after 788a776069 ("bugreport: collect list of populated
hooks", 2020-05-07), the bug report, which is created in our hook-less
test repo, contains an empty section with the enabled hooks. Thus, even
the intention of our helper is a bit misguided: there is nothing
inherently wrong with having an empty section in the bug report.
Let's instead split this test into three: first verify that we generate
a report at all, then check that the introductory blurb looks the way it
should, then verify that the "[System Info]" seems to contain the right
things. (The "[Enabled Hooks]" section is tested later in the script.)
Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
This is a much belated follow-up to v1 [1]. Feedback on that patch
ranged from not bothering checking the generated report at all to
implementing `--no-system-info` and `--no-hooks-info` so that we could
easily test the introductory blurb verbatim.
I hope I'm hitting a reasonable middle ground here. Verifying the
contents at least somewhat is in line with the original intention of
the test. Those `--no-...-info` could probably be useful to bug
reporters, but it feels wrong to me to implement them just to be able
to use them in the tests.
[1] https://lore.kernel.org/git/20210411143354.25134-1-martin.agren@gmail.com/
t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
1 file changed, 44 insertions(+), 23 deletions(-)
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index b6d2f591ac..9390631b17 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -5,29 +5,50 @@ test_description='git bugreport'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
-# Headers "[System Info]" will be followed by a non-empty line if we put some
-# information there; we can make sure all our headers were followed by some
-# information to check if the command was successful.
-HEADER_PATTERN="^\[.*\]$"
-
-check_all_headers_populated () {
- while read -r line
- do
- if test "$(grep "$HEADER_PATTERN" "$line")"
- then
- echo "$line"
- read -r nextline
- if test -z "$nextline"; then
- return 1;
- fi
- fi
- done
-}
-
-test_expect_success 'creates a report with content in the right places' '
- test_when_finished rm git-bugreport-check-headers.txt &&
- git bugreport -s check-headers &&
- check_all_headers_populated <git-bugreport-check-headers.txt
+test_expect_success 'create a report' '
+ git bugreport -s format &&
+ test_file_not_empty git-bugreport-format.txt
+'
+
+test_expect_success 'report contains wanted template (before first section)' '
+ awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual &&
+ cat >expect <<-\EOF &&
+ Thank you for filling out a Git bug report!
+ Please answer the following questions to help us understand your issue.
+
+ What did you do before the bug happened? (Steps to reproduce your issue)
+
+ What did you expect to happen? (Expected behavior)
+
+ What happened instead? (Actual behavior)
+
+ What'\''s different between what you expected and what actually happened?
+
+ Anything else you want to add:
+
+ Please review the rest of the bug report below.
+ You can delete any lines you don'\''t wish to share.
+
+
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'sanity check "System Info" section' '
+ test_when_finished rm -f git-bugreport-format.txt &&
+
+ sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system &&
+
+ # The beginning should match "git version --build-info" verbatim,
+ # but rather than checking bit-for-bit equality, just test some basics.
+ grep "git version [0-9]." system &&
+ grep "shell-path: ." system &&
+
+ # After the version, there should be some more info.
+ # This is bound to differ from environment to environment,
+ # so we just do some rather high-level checks.
+ grep "uname: ." system &&
+ grep "compiler info: ." system
'
test_expect_success 'dies if file with same name as report already exists' '
--
2.41.0.404.g5b50783d6b
^ permalink raw reply related
* [PATCH] ref-filter: handle nested tags in --points-at option
From: Jan Klötzke @ 2023-07-01 20:57 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Steve Kemp, René Scharfe,
Stefan Beller
Tags are dereferenced until reaching a different object type to handle
nested tags, e.g. on checkout. In contrast, "git tag --points-at=..."
fails to list such nested tags because only one level of indirection is
obtained in filter_refs(). Implement the recursive dereferencing for the
"--points-at" option when filtering refs to unify the behaviour.
Signed-off-by: Jan Klötzke <jan@kloetzke.net>
---
ref-filter.c | 16 +++++++---------
t/t6302-for-each-ref-filter.sh | 2 ++
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index e0d03a9f8e..ad7f244414 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2211,10 +2211,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
* of oids. If the given ref is a tag, check if the given tag points
* at one of the oids in the given oid array.
* NEEDSWORK:
- * 1. Only a single level of indirection is obtained, we might want to
- * change this to account for multiple levels (e.g. annotated tags
- * pointing to annotated tags pointing to a commit.)
- * 2. As the refs are cached we might know what refname peels to without
+ * As the refs are cached we might know what refname peels to without
* the need to parse the object via parse_object(). peel_ref() might be a
* more efficient alternative to obtain the pointee.
*/
@@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
const struct object_id *oid,
const char *refname)
{
- const struct object_id *tagged_oid = NULL;
struct object *obj;
if (oid_array_lookup(points_at, oid) >= 0)
return oid;
obj = parse_object(the_repository, oid);
+ while (obj && obj->type == OBJ_TAG) {
+ oid = get_tagged_oid((struct tag *)obj);
+ if (oid_array_lookup(points_at, oid) >= 0)
+ return oid;
+ obj = parse_object(the_repository, oid);
+ }
if (!obj)
die(_("malformed object at '%s'"), refname);
- if (obj->type == OBJ_TAG)
- tagged_oid = get_tagged_oid((struct tag *)obj);
- if (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)
- return tagged_oid;
return NULL;
}
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 1ce5f490e9..af223e44d6 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -45,6 +45,8 @@ test_expect_success 'check signed tags with --points-at' '
sed -e "s/Z$//" >expect <<-\EOF &&
refs/heads/side Z
refs/tags/annotated-tag four
+ refs/tags/doubly-annotated-tag An annotated tag
+ refs/tags/doubly-signed-tag A signed tag
refs/tags/four Z
refs/tags/signed-tag four
EOF
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch
From: Jens Lideström @ 2023-07-02 11:50 UTC (permalink / raw)
To: Johannes Sixt, Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], git
In-Reply-To: <e745078f-fa24-6c10-4134-078e7e3f214c@kdbg.org>
> IMO a selection dialog is the second-best solution. The best one is to
> make the branch labels selectable somehow via keyboard navigation. I am
> not a fan of the here implement behavior, because it is only a
> half-solution.
I added a branch selection dialog to the remove command, triggered when there is more than one branch on the selected commit. It works fine and is convenient to use. However, the implementation is more complicated and the patch is much larger.
Since the patch is larger I felt that it was warranted to clarify some variable names.
The checkout command also has got a branch selection dialog.
Create and remove commands have been put in separate commits.
/Jens
On 2023-06-28 22:30, Johannes Sixt wrote:
> Am 28.06.23 um 09:12 schrieb Jens Lideström:
>> My intention is to always remove the first branch head that is displayed
>> for a single commit in the GUI. This caters to the common use case, with
>> only one branch for a single commit. If there are multiple branch heads
>> on a commit and the users don't want to remove the first one then they
>> need to use the mouse context menu to choose which one to delete.
>>
>> I could change the implementation to display a dialog that lets the user
>> choose in case of multiple branch heads.
>
> IMO a selection dialog is the second-best solution. The best one is to
> make the branch labels selectable somehow via keyboard navigation. I am
> not a fan of the here implement behavior, because it is only a
> half-solution.
>
> Also, the order of branch labels on a line is not 100% deterministic:
> when you create a branch, it goes last, but when you refresh the view,
> branch labels are sorted alphabetically (I think). This means you can't
> create a branch and delete it right away if there is already a branch on
> the commit.
>
>> In that case, should I do that as part of this PR, or as a follow up? I
>> would prefer to finish this one first.
>
> My preference would be to not implement this behavior until it is clear
> what it should be.
>
> -- Hannes
>
^ permalink raw reply
* Re: [PATCH 9/9] gitk: default select reset hard in dialog
From: Jens Lideström @ 2023-07-02 12:09 UTC (permalink / raw)
To: Johannes Sixt, Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], git
In-Reply-To: <b2e0d8fa1b54d9dbc6a0831d53acb85f@lidestrom.se>
I updated the implementation to use "mixed" as the default, but allow switching selected item with up and down keys, and accept with enter. It works fine! :)
/Jens
On 2023-06-28 09:16, Jens Lideström wrote:
>> I would prefer to keep the default at "mixed" mode, set the focus on the
>> radio button to make it easy to switch to "hard" mode by hitting the
>> Down arrow key, and then make it so that Enter triggers the OK button.
>
> That indeed sounds better! Safer but still convenient. I'll change the solution
> to this.
>
> I noticed that some dialogues have a key bind to close with success using Enter.
>
> /Jens
>
>
> On 2023-06-28 07:46, Johannes Sixt wrote:
>> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>>> From: Jens Lidestrom <jens@lidestrom.se>
>>>
>>> Reset hard is dangerous but also the most common reset type, and not
>>> having it pre-selected in the dialog is annoying to users.
>>
>> I agree that the operation of the Reset dialog is clumsy before this
>> series. However, this patch together with the previous patch turns it
>> into a foot gun. It becomes far too easy to destroy uncommitted work.
>>
>> I would prefer to keep the default at "mixed" mode, set the focus on the
>> radio button to make it easy to switch to "hard" mode by hitting the
>> Down arrow key, and then make it so that Enter triggers the OK button.
>>
>>> It is also less dangerous in the GUI where there is a confirmation
>>> dialog. Also, dangling commits remain in the GUI and can be recovered.
>>
>> The problem with "hard" mode are not the commits. The real danger is
>> that it blows away uncommitted changes. Besides of that, I do not
>> consider this UI a confirmation dialog.
>>
>> -- Hannes
>>
>>>
>>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>>> ---
>>> gitk-git/gitk | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>>> index 9d93053e360..5b0a0ea46be 100755
>>> --- a/gitk-git/gitk
>>> +++ b/gitk-git/gitk
>>> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
>>> [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id
>>> 1]]
>>> pack $w.m -side top -fill x -padx 20 -pady 20
>>> ${NS}::labelframe $w.f -text [mc "Reset type:"]
>>> - set resettype mixed
>>> + # Reset hard is dangerous but also the most common reset type, and not
>>> + # having it pre-selected in the dialog is annoying to users.
>>> + set resettype hard
>>> ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>>> -text [mc "Soft: Leave working tree and index untouched"]
>>> grid $w.f.soft -sticky w
^ permalink raw reply
* Re: [PATCH 5/9] gitk: add keyboard bind for checkout
From: Jens Lideström @ 2023-07-02 12:10 UTC (permalink / raw)
To: Jens Lidestrom via GitGitGadget, git; +Cc: Paul Mackerras [ ]
In-Reply-To: <53e5a2e40abbf81e5577b2f79588bd8b0be6e923.1687876885.git.gitgitgadget@gmail.com>
I added a branch selection dialog to be be able to check out any branch on a selected commit, in the same way as for the remove branch command.
/Jens
On 2023-06-27 16:41, Jens Lidestrom via GitGitGadget wrote:
> From: Jens Lidestrom <jens@lidestrom.se>
>
> This also introduces the ability to check out detatched heads. This
> shouldn't result any problems, because gitk already works with
> detatched heads if they are checked out using the terminal.
>
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
> gitk-git/gitk | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index bfe912983f4..596977abe89 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2691,6 +2691,7 @@ proc makewindow {} {
> bind $ctext <Button-1> {focus %W}
> bind $ctext <<Selection>> rehighlight_search_results
> bind . <$M1B-t> {resethead [selected_line_id]}
> + bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
> for {set i 1} {$i < 10} {incr i} {
> bind . <$M1B-Key-$i> [list go_to_parent $i]
> }
> @@ -2707,7 +2708,7 @@ proc makewindow {} {
> {mc "Create tag" command mktag}
> {mc "Copy commit reference" command copyreference}
> {mc "Write commit to file" command writecommit}
> - {mc "Create new branch" command mkbranch}
> + {mc "Create new branch" command {mkbranch $rowmenuid}}
> {mc "Cherry-pick this commit" command cherrypick}
> {mc "Reset current branch to here" command {resethead $rowmenuid}}
> {mc "Mark this commit" command markhere}
> @@ -2732,7 +2733,7 @@ proc makewindow {} {
>
> set headctxmenu .headctxmenu
> makemenu $headctxmenu {
> - {mc "Check out this branch" command cobranch}
> + {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
> {mc "Rename this branch" command mvbranch}
> {mc "Remove this branch" command rmbranch}
> {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
> @@ -3183,6 +3184,7 @@ proc keys {} {
> [mc "<%s-minus> Decrease font size" $M1T]
> [mc "<F5> Update"]
> [mc "<%s-T> Reset current branch to selected commit" $M1T]
> +[mc "<%s-O> Check out selected commit" $M1T]
> " \
> -justify left -bg $bgcolor -border 2 -relief groove
> pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -9978,25 +9980,26 @@ proc headmenu {x y id head} {
> tk_popup $headctxmenu $x $y
> }
>
> -proc cobranch {} {
> - global headmenuid headmenuhead headids
> +proc checkout {newhead newheadid} {
> + global headids
> global showlocalchanges
>
> # check the tree is clean first??
> - set newhead $headmenuhead
> +
> + # The ref is either the head, if it exists, or the ID
> + set newheadref [expr {$newhead ne "" ? $newhead : $newheadid}]
> +
> set command [list | git checkout]
> if {[string match "remotes/*" $newhead]} {
> set remote $newhead
> set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
> - # The following check is redundant - the menu option should
> - # be disabled to begin with...
> if {[info exists headids($newhead)]} {
> error_popup [mc "A local branch named %s exists already" $newhead]
> return
> }
> lappend command -b $newhead --track $remote
> } else {
> - lappend command $newhead
> + lappend command $newheadref
> }
> lappend command 2>@1
> nowbusy checkout [mc "Checking out"]
> @@ -10011,11 +10014,11 @@ proc cobranch {} {
> dodiffindex
> }
> } else {
> - filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
> + filerun $fd [list readcheckoutstat $fd $newhead $newheadref $newheadid]
> }
> }
>
> -proc readcheckoutstat {fd newhead newheadid} {
> +proc readcheckoutstat {fd newhead newheadref newheadid} {
> global mainhead mainheadid headids idheads showlocalchanges progresscoords
> global viewmainheadid curview
>
> @@ -10034,12 +10037,13 @@ proc readcheckoutstat {fd newhead newheadid} {
> return
> }
> set oldmainid $mainheadid
> - if {! [info exists headids($newhead)]} {
> +
> + if {$newhead ne "" && ! [info exists headids($newhead)]} {
> set headids($newhead) $newheadid
> lappend idheads($newheadid) $newhead
> addedhead $newheadid $newhead
> }
> - set mainhead $newhead
> + set mainhead $newheadref
> set mainheadid $newheadid
> set viewmainheadid($curview) $newheadid
> redrawtags $oldmainid
^ permalink raw reply
* Re: [PATCH 0/9] gitk: improve keyboard support
From: Jens Lideström @ 2023-07-02 12:28 UTC (permalink / raw)
To: Johannes Sixt, Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], git
In-Reply-To: <0cb94aa5-726f-a57f-858c-b29764c63ce7@kdbg.org>
I have updated the PR after suggestions from Hannes. Mainly these changes have been made:
* The reset command dialog uses "mixed" as the default, but is more convenient to navigate with the keyboard.
* Remove and checkout branch commands now have branch selection dialogs if there is more than one branch head on the selected commit.
* Remove and checkout branch command patches handles a few more cases regarding remote branches and detached heads that I didn't think about originally. This has made them larger.
* I have split one commit, added another and moved some functionality around. Because of this the original patch number are no longer in sync with GitHub. How should I handle that?
On 2023-06-28 08:09, Johannes Sixt wrote:
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
> would be for Paul to integrate patches that were generated by gitgitgadget).
@Paul Mackerras: Paul, can you have a look at this? Can you accept a PR through GitGitGadget if I rebase it onto master for git.ozlabs.org/~paulus/gitk? Or do you have some other preferred way to receive patches?
Best regards,
Jens Lideström
On 2023-06-28 08:09, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> It is often convenient to use the keyboard to navigate the gitk GUI and
>> there are keyboard shortcut bindings for many operations such as searching
>> and scrolling. There is however no keyboard binding for the most common
>> operations on branches and commits: Check out, reset, cherry-pick, create
>> and delete branches.
>>
>> This PR adds keyboard bindings for these 5 commands. It also adjusts some
>> GUI focus defaults to simplify keyboard navigation.
>>
>> Some refactoring of the command implementation has been necessary.
>> Originally the commands was using the mouse context menu to get info about
>> the head and commit to act on. When using keyboard binds this information
>> isn't available so instead the row that is selected in the GUI is used. By
>> adding procedures for doing this the PR lays the groundwork for more similar
>> keyboard binds in the future.
>
> I like it when an application can be navigated with the keyboard. These
> changes are very much appreciated.
>
> I've left some comments on individual commits. The important one is that
> I think it makes the Reset dialog way too easy to destroy uncommitted work.
>
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
> would be for Paul to integrate patches that were generated by gitgitgadget).
>
> -- Hannes
>
>>
>> I'm including Paul Mackerras because he seems to be the maintainer of gitk.
>> Can you review, Paul?
>>
>> Jens Lidestrom (9):
>> gitk: add procedures to get commit info from selected row
>> gitk: use term "current branch" in gui
>> gitk: add keyboard bind for reset
>> gitk: show branch name in reset dialog
>> gitk: add keyboard bind for checkout
>> gitk: add keyboard bind for create and remove branch
>> gitk: add keyboard bind to cherry-pick
>> gitk: focus ok button in reset dialog
>> gitk: default select reset hard in dialog
>>
>> gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 96 insertions(+), 36 deletions(-)
>>
>>
>> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1551
>
^ permalink raw reply
* Re: [PATCH] ref-filter: handle nested tags in --points-at option
From: Jeff King @ 2023-07-02 12:56 UTC (permalink / raw)
To: Jan Klötzke
Cc: git, Junio C Hamano, Steve Kemp, René Scharfe, Stefan Beller
In-Reply-To: <20230701205703.1172505-1-jan@kloetzke.net>
On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:
> Tags are dereferenced until reaching a different object type to handle
> nested tags, e.g. on checkout. In contrast, "git tag --points-at=..."
> fails to list such nested tags because only one level of indirection is
> obtained in filter_refs(). Implement the recursive dereferencing for the
> "--points-at" option when filtering refs to unify the behaviour.
That seems reasonable to me. It is changing the definition of
--points-at slightly, but I think in a way that should be less
surprising to users (i.e., we can consider the old behavior a bug). The
existing documentation is sufficiently vague about "points" that I don't
think it needs to be updated (though arguably we could improve that
here, too).
Note that most other tag-peeling in Git (like the peeled values returned
by upload-pack) errs in the opposite direction: they peel completely,
and don't show the intermediate values. We _could_ switch to that here,
but I think it would be a behavior regression (but see below on why we
might entertain the thought).
My biggest question would be whether this introduces any performance
penalty for the more common cases (lightweight tags and single-level
annotated tags). The answer is "no", I think; we are already paying the
cost to parse every object to find out if it's a tag, and your new loop
only does an extra parse if we see a tag-of-tag. Good.
Let me go off on a tangent here, since I'm looking at the performance
of this function. The current code is already rather pessimal here, as
we could probably avoid parsing non-tags entirely. Some strategies
there are:
1. We could check the object type via oid_object_info() before
parsing. This carries a small penalty (two lookups) for tags but
a big win (avoiding loading the object contents) for non-tags.
An easy way to do this is to replace the parse_object() with
parse_object_with_flags(PARSE_OBJECT_SKIP_HASH_CHECK), which
tries to avoid loading object contents (especially using the
commit-graph for commits, which presumably covers most non-tag
refs).
2. We could be using the peel_iterated_oid() interface (this is the
peel_ref() thing mentioned in the comment you touched, but it has
since been renamed). But it does the "peel all the way" thing
mentioned above (both because of its interface, but also because
that's what the packed-refs peel lines store).
So to do that we'd either have to enhance the packed-refs store
(which would not be too hard to do in a backwards-compatible
way), or switch --points-at to only match either the direct ref
value or the fully-peeled value.
I don't think either of those is something your patch needs to deal
with. It is not making these kinds of optimizations any harder (it is
the existing "peel only once" behavior that does so). I mostly wanted
to get it written down while we are all looking at this function.
> diff --git a/ref-filter.c b/ref-filter.c
> index e0d03a9f8e..ad7f244414 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2211,10 +2211,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
> * of oids. If the given ref is a tag, check if the given tag points
> * at one of the oids in the given oid array.
> * NEEDSWORK:
> - * 1. Only a single level of indirection is obtained, we might want to
> - * change this to account for multiple levels (e.g. annotated tags
> - * pointing to annotated tags pointing to a commit.)
> - * 2. As the refs are cached we might know what refname peels to without
> + * As the refs are cached we might know what refname peels to without
> * the need to parse the object via parse_object(). peel_ref() might be a
> * more efficient alternative to obtain the pointee.
> */
Great, thanks for cleaning up this comment.
> @@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
> const struct object_id *oid,
> const char *refname)
> {
> - const struct object_id *tagged_oid = NULL;
> struct object *obj;
>
> if (oid_array_lookup(points_at, oid) >= 0)
> return oid;
> obj = parse_object(the_repository, oid);
> + while (obj && obj->type == OBJ_TAG) {
> + oid = get_tagged_oid((struct tag *)obj);
> + if (oid_array_lookup(points_at, oid) >= 0)
> + return oid;
> + obj = parse_object(the_repository, oid);
> + }
OK, so we are doing the usual peeling loop here. I wondered if we might
be able to use peel_object(), but it again suffers from the "peel all
the way" syndrome. So we have to loop ourselves so that we can check at
each level. Good.
> if (!obj)
> die(_("malformed object at '%s'"), refname);
This will now trigger if refname points to a broken object, or if its
tag does. I think the resulting message is OK in either case (and
presumably lower level code would produce extra error messages, too).
> - if (obj->type == OBJ_TAG)
> - tagged_oid = get_tagged_oid((struct tag *)obj);
> - if (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)
> - return tagged_oid;
This code is moved into the loop body, but your version there drops the
"if (tagged_oid)" check. I think that is OK (and even preferable),
though. In get_tagged_oid() we will die() if the tagged object is NULL
(though even before switching to that function this check was
questionable, because it is "tag->tagged" that may be NULL, and we were
dereferencing that unconditionally).
So the code looks good.
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 1ce5f490e9..af223e44d6 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -45,6 +45,8 @@ test_expect_success 'check signed tags with --points-at' '
> sed -e "s/Z$//" >expect <<-\EOF &&
> refs/heads/side Z
> refs/tags/annotated-tag four
> + refs/tags/doubly-annotated-tag An annotated tag
> + refs/tags/doubly-signed-tag A signed tag
> refs/tags/four Z
> refs/tags/signed-tag four
> EOF
And the test looks good, too. It is nice that we can rely on the
existing setup for the doubly-* tags.
Thanks for an easy-to-review patch. :)
-Peff
^ permalink raw reply
* Re: [PATCH] repack: only repack .packs that exist
From: Jeff King @ 2023-07-02 13:11 UTC (permalink / raw)
To: Taylor Blau; +Cc: Derrick Stolee via GitGitGadget, git, gitster, Derrick Stolee
In-Reply-To: <ZJ1N2I6sDfxhrJo8@nand.local>
On Thu, Jun 29, 2023 at 05:24:40AM -0400, Taylor Blau wrote:
> > I also kind of wonder if this repack code should simply be loading and
> > iterating the packed_git list, but that is a much bigger change.
>
> I have wanted to do this for a while ;-). The minimal patch is less
> invasive than I had thought:
Yeah, I agree it's not too bad. If we want to go that route, though, I
think we should do it on top of Stolee's patch, though (which makes it a
pure cleanup once the behaviors are aligned).
I'm also not sure if you'd need to do anything tricky with alternate
object dirs (it looks like the existing code ignores them entirely, so I
guess we'd want to skip entries without pack_local set).
> [...]
> I think you could probably go further than this, since having to store
> the suffix-less pack names in the fname_kept and fname_nonkept lists is
> kind of weird.
>
> It would be nice if we could store pointers to the actual packed_git
> structs themselves in place of those lists instead, but I'm not
> immediately sure how feasible it would be to do since we re-prepare the
> object store between enumerating and then removing these packs.
I think that would work, because we do not ever drop packed_git entries
from the list (even if the files were deleted between prepare/reprepare).
But it also creates a subtle memory ownership dependency/assumption
between the two bits of code. It seems clearer to me to just copy the
names out to our own lists here (i.e., the patch you showed).
-Peff
^ permalink raw reply
* [PATCH] docs: add necessary headers to Documentation/MFOW.txt
From: Vinayak Dev @ 2023-07-02 15:14 UTC (permalink / raw)
To: gitster; +Cc: git, Vinayak Dev
From: Vinayak Dev <vinayakdev.sci@gmail.com>
The tutorial in Documentation/MyFirstObjectWalk.txt
contains the functions trace_printf(), oid_to_hex(),
and pp_commit_easy(), and struct oidset, which are used
without any hint of where they are defined. When the provided
code is compiled, the compiler returns an error, stating that
the functions and the struct are used before declaration. Therefore,include
necessary header files (the ones which have no mentions in the tutorial).
Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
---
I sent a patch to the mailing list previously, but today I noticed that
the CI builds for the branch on my fork were failing. I turns out that
the tutorial required addition of more files than I had noticed.
I am really, really sorry for this mistake, but I am sure that the tutorial
is fixed now. The CI builds now pass perfectly.
Documentation/MyFirstObjectWalk.txt | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 200e628e30..c68cdb11b9 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -41,6 +41,7 @@ Open up a new file `builtin/walken.c` and set up the command handler:
*/
#include "builtin.h"
+#include "trace.h"
int cmd_walken(int argc, const char **argv, const char *prefix)
{
@@ -49,12 +50,13 @@ int cmd_walken(int argc, const char **argv, const char *prefix)
}
----
-NOTE: `trace_printf()` differs from `printf()` in that it can be turned on or
-off at runtime. For the purposes of this tutorial, we will write `walken` as
-though it is intended for use as a "plumbing" command: that is, a command which
-is used primarily in scripts, rather than interactively by humans (a "porcelain"
-command). So we will send our debug output to `trace_printf()` instead. When
-running, enable trace output by setting the environment variable `GIT_TRACE`.
+NOTE: `trace_printf()`, defined in `trace.h`, differs from `printf()` in
+that it can be turned on or off at runtime. For the purposes of this
+tutorial, we will write `walken` as though it is intended for use as
+a "plumbing" command: that is, a command which is used primarily in
+scripts, rather than interactively by humans (a "porcelain" command).
+So we will send our debug output to `trace_printf()` instead.
+When running, enable trace output by setting the environment variable `GIT_TRACE`.
Add usage text and `-h` handling, like all subcommands should consistently do
(our test suite will notice and complain if you fail to do so).
@@ -341,6 +343,10 @@ the walk loop below the `prepare_revision_walk()` call within your
`walken_commit_walk()`:
----
+#include "pretty.h"
+
+...
+
static void walken_commit_walk(struct rev_info *rev)
{
struct commit *commit;
@@ -754,6 +760,10 @@ reachable objects are walked in order to populate the list.
First, add the `struct oidset` and related items we will use to iterate it:
----
+#include "oidset.h"
+
+...
+
static void walken_object_walk(
...
@@ -805,6 +815,10 @@ just walks of commits. First, we'll make our handlers chattier - modify
go:
----
+#include "hex.h"
+
+...
+
static void walken_show_commit(struct commit *cmt, void *buf)
{
trace_printf("commit: %s\n", oid_to_hex(&cmt->object.oid));
base-commit: 9748a6820043d5815bee770ffa51647e0adc2cf0
--
2.41.0
^ permalink raw reply related
* Who are you
From: ross thomas @ 2023-07-02 15:32 UTC (permalink / raw)
To: git@vger.kernel.org
You on GitHub.?
Sent from my iPhone
^ permalink raw reply
* Re: Who are you
From: Luna Jernberg @ 2023-07-02 16:10 UTC (permalink / raw)
To: ross thomas; +Cc: git@vger.kernel.org
In-Reply-To: <CH2PR07MB7334359246A319C195383C50FB28A@CH2PR07MB7334.namprd07.prod.outlook.com>
https://github.com/bittin
Den sön 2 juli 2023 kl 18:07 skrev ross thomas <rossrecovery93245@outlook.com>:
>
> You on GitHub.?
>
> Sent from my iPhone
^ permalink raw reply
* Re: [PATCH] ref-filter: handle nested tags in --points-at option
From: René Scharfe @ 2023-07-02 16:25 UTC (permalink / raw)
To: Jeff King, Jan Klötzke
Cc: git, Junio C Hamano, Steve Kemp, Stefan Beller
In-Reply-To: <20230702125611.GA1036686@coredump.intra.peff.net>
Am 02.07.23 um 14:56 schrieb Jeff King:
> On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:
>
>> @@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
>> const struct object_id *oid,
>> const char *refname)
>> {
>> - const struct object_id *tagged_oid = NULL;
>> struct object *obj;
>>
>> if (oid_array_lookup(points_at, oid) >= 0)
>> return oid;
>> obj = parse_object(the_repository, oid);
>> + while (obj && obj->type == OBJ_TAG) {
>> + oid = get_tagged_oid((struct tag *)obj);
>> + if (oid_array_lookup(points_at, oid) >= 0)
>> + return oid;
>> + obj = parse_object(the_repository, oid);
>> + }
>
> OK, so we are doing the usual peeling loop here. I wondered if we might
> be able to use peel_object(), but it again suffers from the "peel all
> the way" syndrome. So we have to loop ourselves so that we can check at
> each level. Good.
>
>> if (!obj)
>> die(_("malformed object at '%s'"), refname);
>
> This will now trigger if refname points to a broken object, or if its
> tag does. I think the resulting message is OK in either case (and
> presumably lower level code would produce extra error messages, too).
>
>> - if (obj->type == OBJ_TAG)
>> - tagged_oid = get_tagged_oid((struct tag *)obj);
>> - if (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)
>> - return tagged_oid;
>
> This code is moved into the loop body, but your version there drops the
> "if (tagged_oid)" check. I think that is OK (and even preferable),
> though. In get_tagged_oid() we will die() if the tagged object is NULL
> (though even before switching to that function this check was
> questionable, because it is "tag->tagged" that may be NULL, and we were
> dereferencing that unconditionally).
The check is necessary in the current code because tagged_oid is NULL if
obj is not a tag. The new code no longer needs it.
> So the code looks good.
I agree.
René
^ permalink raw reply
* [PATCH 0/2] advise about force-pushing as an alternative to reconciliation
From: Alex Henrie @ 2023-07-02 20:08 UTC (permalink / raw)
To: git, git, christiwald, john, philipoakley, gitster; +Cc: Alex Henrie
Many times now, I have seen novices do the following:
1. Start work on their own personal topic branch
2. Push the branch to origin
3. Rebase the branch onto origin/master
4. Try to push again, but Git says they need to pull
5. Pull and make a mess trying to reconcile the older topic branch with
the rebased topic branch
Help avoid this mistake by giving advice that mentions force-pushing,
rather than assuming that the user always wants to do reconciliation.
Alex Henrie (2):
remote: advise about force-pushing as an alternative to reconciliation
push: advise about force-pushing as an alternative to reconciliation
builtin/push.c | 22 +++++++++++++---------
remote.c | 3 ++-
2 files changed, 15 insertions(+), 10 deletions(-)
--
2.41.0
^ permalink raw reply
* [PATCH 2/2] push: advise about force-pushing as an alternative to reconciliation
From: Alex Henrie @ 2023-07-02 20:08 UTC (permalink / raw)
To: git, git, christiwald, john, philipoakley, gitster; +Cc: Alex Henrie
In-Reply-To: <20230702200818.1038494-1-alexhenrie24@gmail.com>
Also, don't put `git pull` in an awkward parenthetical, because
`git pull` can always be used to reconcile branches and is the normal
way to do so.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
builtin/push.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 6f8a8dc711..9441c71bb0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -301,21 +301,24 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
static const char message_advice_pull_before_push[] =
N_("Updates were rejected because the tip of your current branch is behind\n"
- "its remote counterpart. Integrate the remote changes (e.g.\n"
- "'git pull ...') before pushing again.\n"
+ "its remote counterpart. Use 'git pull' to integrate the remote changes\n"
+ "before pushing again, or use 'git push --force' to delete the remote\n"
+ "changes and replace them with your own.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
static const char message_advice_checkout_pull_push[] =
N_("Updates were rejected because a pushed branch tip is behind its remote\n"
- "counterpart. Check out this branch and integrate the remote changes\n"
- "(e.g. 'git pull ...') before pushing again.\n"
+ "counterpart. Check out this branch and use 'git pull' to integrate the\n"
+ "remote changes before pushing again, or use 'git push --force' to\n"
+ "delete the remote changes and replace them with your own.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
static const char message_advice_ref_fetch_first[] =
N_("Updates were rejected because the remote contains work that you do\n"
"not have locally. This is usually caused by another repository pushing\n"
- "to the same ref. You may want to first integrate the remote changes\n"
- "(e.g., 'git pull ...') before pushing again.\n"
+ "to the same ref. Use 'git pull' to integrate the remote changes before\n"
+ "pushing again, or use 'git push --force' to delete the remote changes\n"
+ "and replace them with your own.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
static const char message_advice_ref_already_exists[] =
@@ -328,9 +331,10 @@ static const char message_advice_ref_needs_force[] =
static const char message_advice_ref_needs_update[] =
N_("Updates were rejected because the tip of the remote-tracking\n"
- "branch has been updated since the last checkout. You may want\n"
- "to integrate those changes locally (e.g., 'git pull ...')\n"
- "before forcing an update.\n");
+ "branch has been updated since the last checkout. Use 'git pull' to\n"
+ "integrate the remote changes before pushing again, or use\n"
+ "'git push --force' to delete the remote changes and replace them\n"
+ "with your own.\n");
static void advise_pull_before_push(void)
{
--
2.41.0
^ permalink raw reply related
* [PATCH 1/2] remote: advise about force-pushing as an alternative to reconciliation
From: Alex Henrie @ 2023-07-02 20:08 UTC (permalink / raw)
To: git, git, christiwald, john, philipoakley, gitster; +Cc: Alex Henrie
In-Reply-To: <20230702200818.1038494-1-alexhenrie24@gmail.com>
Also, don't imply that `git pull` is only for merging.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
remote.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index a81f2e2f17..161d0cfe96 100644
--- a/remote.c
+++ b/remote.c
@@ -2323,7 +2323,8 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
base, ours, theirs);
if (advice_enabled(ADVICE_STATUS_HINTS))
strbuf_addstr(sb,
- _(" (use \"git pull\" to merge the remote branch into yours)\n"));
+ _(" (use \"git pull\" to reconcile your local branch with the remote branch,\n"
+ " or \"git push --force\" to overwrite the remote branch with your local branch)\n"));
}
free(base);
return 1;
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] ref-filter: handle nested tags in --points-at option
From: Jeff King @ 2023-07-02 20:27 UTC (permalink / raw)
To: René Scharfe
Cc: Jan Klötzke, git, Junio C Hamano, Steve Kemp, Stefan Beller
In-Reply-To: <df912fc4-601b-4d43-c6e4-aa5f74ec031f@web.de>
On Sun, Jul 02, 2023 at 06:25:13PM +0200, René Scharfe wrote:
> >> - if (obj->type == OBJ_TAG)
> >> - tagged_oid = get_tagged_oid((struct tag *)obj);
> >> - if (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)
> >> - return tagged_oid;
> >
> > This code is moved into the loop body, but your version there drops the
> > "if (tagged_oid)" check. I think that is OK (and even preferable),
> > though. In get_tagged_oid() we will die() if the tagged object is NULL
> > (though even before switching to that function this check was
> > questionable, because it is "tag->tagged" that may be NULL, and we were
> > dereferencing that unconditionally).
>
> The check is necessary in the current code because tagged_oid is NULL if
> obj is not a tag. The new code no longer needs it.
Oh, right. Probably:
if (obj->type == OBJ_TAG) {
const struct object_id *tagged_oid = get_tagged_oid((struct tag *)obj);
if (oid_array_lookup(points_at, tagged_oid) >= 0)
return tagged_oid;
}
would have been a better way to write the original, but the new one with
the loop is better still. ;)
I also notice that the function returns a pointer to an oid, even though
the sole caller only cares about a boolean result. Not that big a deal,
though the memory lifetime of the return value is confusing. We might
return "tagged_oid" which points to a struct that will live forever, but
we might also return "oid" directly, which points to memory passed in
from the caller with a limited lifetime.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox