* Re: Question: rerere preimage format
From: Junio C Hamano @ 2023-12-16 17:34 UTC (permalink / raw)
To: Mohamed Mohey; +Cc: git
In-Reply-To: <CAAY9aimx4X2jDaFz+jWtAj==g+J3QA8LKPQFePyGFhVvbwKNtQ@mail.gmail.com>
Mohamed Mohey <mmi9433@gmail.com> writes:
> My question is:
>
> Is there a reason rerere's output is like this that I'm missing? Or is
> it just there for convenience since it doesn't affect rerere's
> intended functionality?
>
>
> Thanks,
> Mohamed Mohey
Documentation/technical/rerere.txt would be a good place to start.
^ permalink raw reply
* [PATCH v3 0/2] fix fetch atomic error message
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702556642.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
# Changes since v2:
Changed the patches with help from Patrick.
# range-diff v2...v3
1: 0b9865f1df ! 1: 0a6c53de7c t5574: test porcelain output of atomic fetch
@@ Commit message
In a later commit, we will fix this issue.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## t/t5574-fetch-output.sh ##
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch porcelain output' '
+ FORCE_UPDATED_NEW=$(git rev-parse HEAD)
'
-+for opt in off on
++for opt in "" "--atomic"
+do
-+ case $opt in
-+ on)
-+ opt=--atomic
-+ ;;
-+ off)
-+ opt=
-+ ;;
-+ esac
+ test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_when_finished "rm -rf porcelain" &&
+
2: e10fa198dd ! 2: a8a7658fb2 fetch: no redundant error message for atomic fetch
@@ Commit message
will appear at the end of do_fetch(). It was introduced in b3a804663c
(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
- In function do_fetch(), a failure message is already shown before the
- retcode is set, so we should not call additional error() at the end of
- this function.
+ Because a failure message is displayed before setting retcode in the
+ function do_fetch(), calling error() on the err message at the end of
+ this function may result in redundant or empty error message to be
+ displayed.
We can remove the redundant error() function, because we know that
the function ref_transaction_abort() never fails. While we can find a
@@ Commit message
if (ref_transaction_abort(transaction, &error))
error("abort: %s", error.buf);
- We can fix this issue follow this pattern, and the test case "fetch
- porcelain output (atomic)" in t5574 will also be fixed. If in the future
- we decide that we don't need to check the return value of the function
- ref_transaction_abort(), this change can be fixed along with it.
+ Following this pattern, we can tolerate the return value of the function
+ ref_transaction_abort() being changed in the future. We also delay the
+ output of the err message to the end of do_fetch() to reduce redundant
+ code. With these changes, the test case "fetch porcelain output
+ (atomic)" in t5574 will also be fixed.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## builtin/fetch.c ##
+@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
+ if (atomic_fetch) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+- retcode = error("%s", err.buf);
++ retcode = -1;
+ goto cleanup;
+ }
+ }
+@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
+
+ retcode = ref_transaction_commit(transaction, &err);
+ if (retcode) {
+- error("%s", err.buf);
+ ref_transaction_free(transaction);
+ transaction = NULL;
+ goto cleanup;
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
}
cleanup:
- if (retcode && transaction) {
- ref_transaction_abort(transaction, &err);
-+ if (retcode && transaction && ref_transaction_abort(transaction, &err))
- error("%s", err.buf);
-- }
+- error("%s", err.buf);
++ if (retcode) {
++ if (err.len) {
++ error("%s", err.buf);
++ strbuf_reset(&err);
++ }
++ if (transaction && ref_transaction_abort(transaction, &err) &&
++ err.len)
++ error("%s", err.buf);
+ }
display_state_release(&display_state);
Jiang Xin (2):
t5574: test porcelain output of atomic fetch
fetch: no redundant error message for atomic fetch
builtin/fetch.c | 14 ++++---
t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------
2 files changed, 59 insertions(+), 44 deletions(-)
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* [PATCH v3 1/2] t5574: test porcelain output of atomic fetch
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702821462.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
The test case "fetch porcelain output" checks output of the fetch
command. The error output must be empty with the follow assertion:
test_must_be_empty stderr
But this assertion fails if using atomic fetch. Refactor this test case
to use different fetch options by splitting it into three test cases.
1. "setup for fetch porcelain output".
2. "fetch porcelain output": for non-atomic fetch.
3. "fetch porcelain output (atomic)": for atomic fetch.
Add new command "test_commit ..." in the first test case, so that if we
run these test cases individually (--run=4-6), "git rev-parse HEAD~"
command will work properly. Run the above test cases, we can find that
one test case has a known breakage, as shown below:
ok 4 - setup for fetch porcelain output
ok 5 - fetch porcelain output # TODO known breakage vanished
not ok 6 - fetch porcelain output (atomic) # TODO known breakage
The failed test case has an error message with only the error prompt but
no message body, as follows:
'stderr' is not empty, it contains:
error:
In a later commit, we will fix this issue.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/t5574-fetch-output.sh | 89 +++++++++++++++++++++++------------------
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 90e6dcb9a7..b579364c47 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -61,11 +61,10 @@ test_expect_success 'fetch compact output' '
test_cmp expect actual
'
-test_expect_success 'fetch porcelain output' '
- test_when_finished "rm -rf porcelain" &&
-
+test_expect_success 'setup for fetch porcelain output' '
# Set up a bunch of references that we can use to demonstrate different
# kinds of flag symbols in the output format.
+ test_commit commit-for-porcelain-output &&
MAIN_OLD=$(git rev-parse HEAD) &&
git branch "fast-forward" &&
git branch "deleted-branch" &&
@@ -74,15 +73,10 @@ test_expect_success 'fetch porcelain output' '
FORCE_UPDATED_OLD=$(git rev-parse HEAD) &&
git checkout main &&
- # Clone and pre-seed the repositories. We fetch references into two
- # namespaces so that we can test that rejected and force-updated
- # references are reported properly.
- refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
- git clone . porcelain &&
- git -C porcelain fetch origin $refspecs &&
+ # Backup to preseed.git
+ git clone --mirror . preseed.git &&
- # Now that we have set up the client repositories we can change our
- # local references.
+ # Continue changing our local references.
git branch new-branch &&
git branch -d deleted-branch &&
git checkout fast-forward &&
@@ -91,36 +85,53 @@ test_expect_success 'fetch porcelain output' '
git checkout force-updated &&
git reset --hard HEAD~ &&
test_commit --no-tag force-update-new &&
- FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
-
- cat >expect <<-EOF &&
- - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
- - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
- ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
- * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
- + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
- * $ZERO_OID $MAIN_OLD refs/forced/new-branch
- $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
- + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
- * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
- EOF
-
- # Execute a dry-run fetch first. We do this to assert that the dry-run
- # and non-dry-run fetches produces the same output. Execution of the
- # fetch is expected to fail as we have a rejected reference update.
- test_must_fail git -C porcelain fetch \
- --porcelain --dry-run --prune origin $refspecs >actual &&
- test_cmp expect actual &&
-
- # And now we perform a non-dry-run fetch.
- test_must_fail git -C porcelain fetch \
- --porcelain --prune origin $refspecs >actual 2>stderr &&
- test_cmp expect actual &&
- test_must_be_empty stderr
+ FORCE_UPDATED_NEW=$(git rev-parse HEAD)
'
+for opt in "" "--atomic"
+do
+ test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_when_finished "rm -rf porcelain" &&
+
+ # Clone and pre-seed the repositories. We fetch references into two
+ # namespaces so that we can test that rejected and force-updated
+ # references are reported properly.
+ refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
+ git clone preseed.git porcelain &&
+ git -C porcelain fetch origin $opt $refspecs &&
+
+ cat >expect <<-EOF &&
+ - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
+ - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
+ ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+ * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
+ + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
+ * $ZERO_OID $MAIN_OLD refs/forced/new-branch
+ $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
+ + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+ * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+ EOF
+
+ # Change the URL of the repository to fetch different references.
+ git -C porcelain remote set-url origin .. &&
+
+ # Execute a dry-run fetch first. We do this to assert that the dry-run
+ # and non-dry-run fetches produces the same output. Execution of the
+ # fetch is expected to fail as we have a rejected reference update.
+ test_must_fail git -C porcelain fetch $opt \
+ --porcelain --dry-run --prune origin $refspecs >actual &&
+ test_cmp expect actual &&
+
+ # And now we perform a non-dry-run fetch.
+ test_must_fail git -C porcelain fetch $opt \
+ --porcelain --prune origin $refspecs >actual 2>stderr &&
+ test_cmp expect actual &&
+ test_must_be_empty stderr
+ '
+done
+
test_expect_success 'fetch porcelain with multiple remotes' '
test_when_finished "rm -rf porcelain" &&
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v3 2/2] fetch: no redundant error message for atomic fetch
From: Jiang Xin @ 2023-12-17 14:11 UTC (permalink / raw)
To: Git List, Junio C Hamano, Patrick Steinhardt; +Cc: Jiang Xin
In-Reply-To: <cover.1702821462.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
If an error occurs during an atomic fetch, a redundant error message
will appear at the end of do_fetch(). It was introduced in b3a804663c
(fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
Because a failure message is displayed before setting retcode in the
function do_fetch(), calling error() on the err message at the end of
this function may result in redundant or empty error message to be
displayed.
We can remove the redundant error() function, because we know that
the function ref_transaction_abort() never fails. While we can find a
common pattern for calling ref_transaction_abort() by running command
"git grep -A1 ref_transaction_abort", e.g.:
if (ref_transaction_abort(transaction, &error))
error("abort: %s", error.buf);
Following this pattern, we can tolerate the return value of the function
ref_transaction_abort() being changed in the future. We also delay the
output of the err message to the end of do_fetch() to reduce redundant
code. With these changes, the test case "fetch porcelain output
(atomic)" in t5574 will also be fixed.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
builtin/fetch.c | 14 +++++++++-----
t/t5574-fetch-output.sh | 2 +-
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d..a284b970ef 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,
if (atomic_fetch) {
transaction = ref_transaction_begin(&err);
if (!transaction) {
- retcode = error("%s", err.buf);
+ retcode = -1;
goto cleanup;
}
}
@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,
retcode = ref_transaction_commit(transaction, &err);
if (retcode) {
- error("%s", err.buf);
ref_transaction_free(transaction);
transaction = NULL;
goto cleanup;
@@ -1775,9 +1774,14 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- if (retcode && transaction) {
- ref_transaction_abort(transaction, &err);
- error("%s", err.buf);
+ if (retcode) {
+ if (err.len) {
+ error("%s", err.buf);
+ strbuf_reset(&err);
+ }
+ if (transaction && ref_transaction_abort(transaction, &err) &&
+ err.len)
+ error("%s", err.buf);
}
display_state_release(&display_state);
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index b579364c47..1400ef14cd 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -90,7 +90,7 @@ test_expect_success 'setup for fetch porcelain output' '
for opt in "" "--atomic"
do
- test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
+ test_expect_success "fetch porcelain output ${opt:+(atomic)}" '
test_when_finished "rm -rf porcelain" &&
# Clone and pre-seed the repositories. We fetch references into two
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 0/3] Sideband-all demultiplexer fixes
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
# Changes since v3:
Change commit message and comments to make them clear for review.
# range-diff v3...v4
1: e387088da2 ! 1: ff4e5aff2a test-pkt-line: add option parser for unpack-sideband
@@ Commit message
* Use "test-tool pkt-line send-split-sideband" to generate sideband
messages.
- * We can pipe these generated sideband messages to command "test-tool
- pkt-line unpack-sideband" to test packet_reader_read() function.
+ * Pipe these generated sideband messages to command "test-tool pkt-line
+ unpack-sideband" to test packet_reader_read() function.
In order to make a complete test of the packet_reader_read() function,
add option parser for command "test-tool pkt-line unpack-sideband".
- To remove newlines in sideband messages, we can use:
+ * To remove newlines in sideband messages, we can use:
- $ test-tool pkt-line unpack-sideband --chomp-newline
+ $ test-tool pkt-line unpack-sideband --chomp-newline
- To preserve newlines in sideband messages, we can use:
+ * To preserve newlines in sideband messages, we can use:
- $ test-tool pkt-line unpack-sideband --no-chomp-newline
+ $ test-tool pkt-line unpack-sideband --no-chomp-newline
- To parse sideband messages using "demultiplex_sideband()" inside the
- function "packet_reader_read()", we can use:
+ * To parse sideband messages using "demultiplex_sideband()" inside the
+ function "packet_reader_read()", we can use:
- $ test-tool pkt-line unpack-sideband --reader-use-sideband
+ $ test-tool pkt-line unpack-sideband --reader-use-sideband
- Add several new test cases in t0070. Among these test cases, we pipe
+ We also add new example sideband packets in send_split_sideband() and
+ add several new test cases in t0070. Among these test cases, we pipe
output of the "send-split-sideband" subcommand to the "unpack-sideband"
subcommand. We found two issues:
1. The two splitted sideband messages "Hello," and " world!\n" should
- be concatenated together. But when we enabled the function
- "demultiplex_sideband()" to parse sideband messages, the first part
- of the splitted message ("Hello,") is lost.
+ be concatenated together. But when we turn on use_sideband field of
+ reader to parse sideband messages, the first part of the splitted
+ message ("Hello,") is lost.
2. The newline characters in sideband 2 (progress info) and sideband 3
- (error message) should be preserved, but they are also trimmed.
+ (error message) should be preserved, but they are both trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## t/helper/test-pkt-line.c ##
@@
@@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
+ /*
+ * When the "use_sideband" field of the reader is turned
+ * on, sideband packets other than the payload have been
-+ * parsed and consumed.
++ * parsed and consumed in packet_reader_read(), and only
++ * the payload arrives here.
+ */
+ if (reader.use_sideband) {
+ write_or_die(1, reader.line, reader.pktlen - 1);
@@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
packet_response_end(1);
+ /*
-+ * The unpack_sideband() function above requires a flush
-+ * packet to end parsing.
++ * We use unpack_sideband() to consume packets. A flush packet
++ * is required to end parsing.
+ */
+ packet_flush(1);
+
@@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
+ test_cmp expect-err err
+'
+
-+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
@@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
+ test_cmp expect-err err
+'
+
-+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
2: 633bfbac39 ! 2: 5942b74cab pkt-line: memorize sideband fragment in reader
@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: --chomp-newline (d
test_cmp expect-err err
'
--test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
-+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
3: 2a2da65fac ! 3: dd2e34da16 pkt-line: do not chomp newlines for sideband messages
@@ pkt-line.h: void packet_fflush(FILE *f);
/*
## t/t0070-fundamental.sh ##
-@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
+@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
test_cmp expect-err err
'
--test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
-+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
Jiang Xin (3):
test-pkt-line: add option parser for unpack-sideband
pkt-line: memorize sideband fragment in reader
pkt-line: do not chomp newlines for sideband messages
pkt-line.c | 36 ++++++++++++++++++++----
pkt-line.h | 4 +++
t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++
4 files changed, 147 insertions(+), 10 deletions(-)
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* [PATCH v4 1/3] test-pkt-line: add option parser for unpack-sideband
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We can use the test helper program "test-tool pkt-line" to test pkt-line
related functions. E.g.:
* Use "test-tool pkt-line send-split-sideband" to generate sideband
messages.
* Pipe these generated sideband messages to command "test-tool pkt-line
unpack-sideband" to test packet_reader_read() function.
In order to make a complete test of the packet_reader_read() function,
add option parser for command "test-tool pkt-line unpack-sideband".
* To remove newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --chomp-newline
* To preserve newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --no-chomp-newline
* To parse sideband messages using "demultiplex_sideband()" inside the
function "packet_reader_read()", we can use:
$ test-tool pkt-line unpack-sideband --reader-use-sideband
We also add new example sideband packets in send_split_sideband() and
add several new test cases in t0070. Among these test cases, we pipe
output of the "send-split-sideband" subcommand to the "unpack-sideband"
subcommand. We found two issues:
1. The two splitted sideband messages "Hello," and " world!\n" should
be concatenated together. But when we turn on use_sideband field of
reader to parse sideband messages, the first part of the splitted
message ("Hello,") is lost.
2. The newline characters in sideband 2 (progress info) and sideband 3
(error message) should be preserved, but they are both trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++
2 files changed, 112 insertions(+), 5 deletions(-)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..6b306cf5ca 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
#include "test-tool.h"
#include "pkt-line.h"
#include "write-or-die.h"
+#include "parse-options.h"
static void pack_line(const char *line)
{
@@ -64,12 +65,33 @@ static void unpack(void)
}
}
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
{
struct packet_reader reader;
- packet_reader_init(&reader, 0, NULL, 0,
- PACKET_READ_GENTLE_ON_EOF |
- PACKET_READ_CHOMP_NEWLINE);
+ int options = PACKET_READ_GENTLE_ON_EOF;
+ int chomp_newline = 1;
+ int reader_use_sideband = 0;
+ const char *const unpack_sideband_usage[] = {
+ "test_tool unpack_sideband [options...]", NULL
+ };
+ struct option cmd_options[] = {
+ OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+ "set use_sideband bit for packet reader (Default: off)"),
+ OPT_BOOL(0, "chomp-newline", &chomp_newline,
+ "chomp newline in packet (Default: on)"),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+ 0);
+ if (argc > 0)
+ usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+ cmd_options);
+
+ if (chomp_newline)
+ options |= PACKET_READ_CHOMP_NEWLINE;
+ packet_reader_init(&reader, 0, NULL, 0, options);
+ reader.use_sideband = reader_use_sideband;
while (packet_reader_read(&reader) != PACKET_READ_EOF) {
int band;
@@ -79,6 +101,17 @@ static void unpack_sideband(void)
case PACKET_READ_EOF:
break;
case PACKET_READ_NORMAL:
+ /*
+ * When the "use_sideband" field of the reader is turned
+ * on, sideband packets other than the payload have been
+ * parsed and consumed in packet_reader_read(), and only
+ * the payload arrives here.
+ */
+ if (reader.use_sideband) {
+ write_or_die(1, reader.line, reader.pktlen - 1);
+ break;
+ }
+
band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
continue; /* skip non-sideband packets */
@@ -97,15 +130,31 @@ static void unpack_sideband(void)
static int send_split_sideband(void)
{
+ const char *foo = "Foo.\n";
+ const char *bar = "Bar.\n";
const char *part1 = "Hello,";
const char *primary = "\001primary: regular output\n";
const char *part2 = " world!\n";
+ /* Each sideband message has a trailing newline character. */
+ send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+ send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+ /*
+ * One sideband message is divided into part1 and part2
+ * by the primary message.
+ */
send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
packet_write(1, primary, strlen(primary));
send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
packet_response_end(1);
+ /*
+ * We use unpack_sideband() to consume packets. A flush packet
+ * is required to end parsing.
+ */
+ packet_flush(1);
+
return 0;
}
@@ -126,7 +175,7 @@ int cmd__pkt_line(int argc, const char **argv)
else if (!strcmp(argv[1], "unpack"))
unpack();
else if (!strcmp(argv[1], "unpack-sideband"))
- unpack_sideband();
+ unpack_sideband(argc - 1, argv + 1);
else if (!strcmp(argv[1], "send-split-sideband"))
send_split_sideband();
else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..297a7f772e 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
test_i18ngrep "missing sideband" err
'
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ cat >expect-err <<-EOF &&
+ Foo.
+ Bar.
+ Hello, world!
+ EOF
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "Foo.Bar.Hello, world!" >expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
test_done
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 2/3] pkt-line: memorize sideband fragment in reader
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When we turn on the "use_sideband" field of the packet_reader,
"packet_reader_read()" will call the function "demultiplex_sideband()"
to parse and consume sideband messages. Sideband fragment which does not
end with "\r" or "\n" will be saved in the sixth parameter "scratch"
and it can be reused and be concatenated when parsing another sideband
message.
In "packet_reader_read()" function, the local variable "scratch" can
only be reused by subsequent sideband messages. But if there is a
payload message between two sideband fragments, the first fragment
which is saved in the local variable "scratch" will be lost.
To solve this problem, we can add a new field "scratch" in
packet_reader to memorize the sideband fragment across different calls
of "packet_reader_read()".
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 5 ++---
pkt-line.h | 3 +++
t/t0070-fundamental.sh | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->options = options;
reader->me = "git";
reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+ strbuf_init(&reader->scratch, 0);
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
- struct strbuf scratch = STRBUF_INIT;
-
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
break;
if (demultiplex_sideband(reader->me, reader->status,
reader->buffer, reader->pktlen, 1,
- &scratch, &sideband_type))
+ &reader->scratch, &sideband_type))
break;
}
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
/* hash algorithm in use */
const struct git_hash_algo *hash_algo;
+
+ /* hold temporary sideband message */
+ struct strbuf scratch;
};
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 297a7f772e..275edbf6e7 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* [PATCH v4 3/3] pkt-line: do not chomp newlines for sideband messages
From: Jiang Xin @ 2023-12-17 14:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When calling "packet_read_with_status()" to parse pkt-line encoded
packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp
newline character for each packet for better line matching. But when
receiving data and progress information using sideband, we should turn
off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline
characters from data and progress information.
When both the server and the client support "sideband-all" capability,
we have a dilemma that newline characters in negotiation packets should
be removed, but the newline characters in the progress information
should be left intact.
Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
to prevent mangling newline characters in sideband messages.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 31 +++++++++++++++++++++++++++++--
pkt-line.h | 1 +
t/t0070-fundamental.sh | 2 +-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..e9061e61a4 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
}
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
- len && buffer[len-1] == '\n')
- len--;
+ len && buffer[len-1] == '\n') {
+ if (options & PACKET_READ_USE_SIDEBAND) {
+ int band = *buffer & 0xff;
+ switch (band) {
+ case 1:
+ /* Chomp newline for payload */
+ len--;
+ break;
+ case 2:
+ case 3:
+ /*
+ * Do not chomp newline for progress and error
+ * message.
+ */
+ break;
+ default:
+ /*
+ * Bad sideband, let's leave it to
+ * demultiplex_sideband() to catch this error.
+ */
+ break;
+ }
+ } else {
+ len--;
+ }
+ }
buffer[len] = 0;
if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
+ if (reader->use_sideband)
+ reader->options |= PACKET_READ_USE_SIDEBAND;
+
/*
* Consume all progress packets until a primary payload packet is
* received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
#define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
#define PACKET_READ_REDACT_URI_PATH (1u<<4)
+#define PACKET_READ_USE_SIDEBAND (1u<<5)
int packet_read(int fd, char *buffer, unsigned size, int options);
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 275edbf6e7..0d2b7d8d93 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply related
* Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording
From: Shreyansh Paliwal @ 2023-12-17 15:07 UTC (permalink / raw)
To: git; +Cc: five231003, gitster, shreyp135
In-Reply-To: <20231203171956.771-1-shreyanshpaliwalcmsmn@gmail.com>
From: shreyp135 <shreyanshpaliwalcmsmn@gmail.com>
ping.
^ permalink raw reply
* Re: End-of-line comments are prompted with "is not a valid attribute name"
From: Junio C Hamano @ 2023-12-17 17:15 UTC (permalink / raw)
To: D无数; +Cc: git
In-Reply-To: <xmqqsf45nbzm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> This is totally expected; nothing to see here.
To put it another way, we do not have "end-of-line comment"
(i.e. the leading part of a line has payload, but the line is
chomped in the middle with a comment character and the remainder of
the line is ignored) at all. We have "commented line" (in other
words, a line that is totally commented out and gets ignored).
I think it is very clearly documented in "git help attributes":
A `gitattributes` file is a simple text file that gives
`attributes` to pathnames.
Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
That is, a pattern followed by an attributes list,
separated by whitespaces. Leading and trailing whitespaces are
ignored. Lines that begin with '#' are ignored. Patterns
that begin with a double quote are quoted in C style.
^ permalink raw reply
* Re: End-of-line comments are prompted with "is not a valid attribute name"
From: D无数 @ 2023-12-17 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7eoiwcb.fsf@gitster.g>
I see, thanks for the answer.
I was fooled by the VSCode highlighting, but also because I didn't try
to figure out the documentation carefully.
Junio C Hamano <gitster@pobox.com> 于2023年12月18日周一 01:15写道：
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > This is totally expected; nothing to see here.
>
> To put it another way, we do not have "end-of-line comment"
> (i.e. the leading part of a line has payload, but the line is
> chomped in the middle with a comment character and the remainder of
> the line is ignored) at all. We have "commented line" (in other
> words, a line that is totally commented out and gets ignored).
>
> I think it is very clearly documented in "git help attributes":
>
> A `gitattributes` file is a simple text file that gives
> `attributes` to pathnames.
>
> Each line in `gitattributes` file is of form:
>
> pattern attr1 attr2 ...
>
> That is, a pattern followed by an attributes list,
> separated by whitespaces. Leading and trailing whitespaces are
> ignored. Lines that begin with '#' are ignored. Patterns
> that begin with a double quote are quoted in C style.
>
^ permalink raw reply
* Unable to install git-2.43.0 from source on macOS Big Sur 11.7.10
From: Jonathan Abrams @ 2023-12-17 22:54 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
I am trying to install git-2.43.0 from source on macOS Big Sur 11.7.10. I have Xcode 13.2.1 installed. I have read https://github.com/git/git/blob/master/INSTALL.
The command that will not complete successfully is sudo make all doc. The Terminal output before the error is as follows.
--
AR libgit.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libgit.a(zlib-uncompress2.o) has no symbols
CC xdiff/xdiffi.o
CC xdiff/xemit.o
CC xdiff/xhistogram.o
CC xdiff/xmerge.o
CC xdiff/xpatience.o
CC xdiff/xprepare.o
CC xdiff/xutils.o
AR xdiff/lib.a
CC reftable/basics.o
CC reftable/error.o
CC reftable/block.o
CC reftable/blocksource.o
CC reftable/iter.o
CC reftable/publicbasics.o
CC reftable/merged.o
CC reftable/pq.o
CC reftable/reader.o
CC reftable/record.o
CC reftable/refname.o
CC reftable/generic.o
CC reftable/stack.o
CC reftable/tree.o
CC reftable/writer.o
AR reftable/libreftable.a
LINK git-daemon
Undefined symbols for architecture x86_64:
"_libiconv", referenced from:
_precompose_utf8_readdir in libgit.a(precompose_utf8.o)
_reencode_string_iconv in libgit.a(utf8.o)
"_libiconv_close", referenced from:
_precompose_string_if_needed in libgit.a(precompose_utf8.o)
_precompose_utf8_closedir in libgit.a(precompose_utf8.o)
_reencode_string_len in libgit.a(utf8.o)
"_libiconv_open", referenced from:
_precompose_string_if_needed in libgit.a(precompose_utf8.o)
_precompose_utf8_opendir in libgit.a(precompose_utf8.o)
_reencode_string_len in libgit.a(utf8.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [git-daemon] Error 1
--
Does anyone reading this know how to resolve this error?
Thank you,
Jonathan S. Abrams
^ permalink raw reply
* Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording
From: Eric Sunshine @ 2023-12-18 0:51 UTC (permalink / raw)
To: Shreyansh Paliwal; +Cc: git, five231003, gitster
In-Reply-To: <20231217153140.1831-1-shreyanshpaliwalcmsmn@gmail.com>
On Sun, Dec 17, 2023 at 10:32 AM Shreyansh Paliwal
<shreyanshpaliwalcmsmn@gmail.com> wrote:
> ping.
Junio was on vacation at the time[1] that this patch was submitted, so
it's quite possible that it simply got overlooked or he hasn't gotten
through the backlog of emails which accumulated while he was away. So,
pinging is indeed the correct thing to do, and the patch is obviously
an improvement, so hopefully it will be picked up soon.
[1]: https://lore.kernel.org/git/xmqq34wj4e55.fsf@gitster.g/
^ permalink raw reply
* Re: Unable to install git-2.43.0 from source on macOS Big Sur 11.7.10
From: Eric Sunshine @ 2023-12-18 1:02 UTC (permalink / raw)
To: Jonathan Abrams; +Cc: git@vger.kernel.org
In-Reply-To: <IA1PR12MB604488A775E3390E9B7E56BEB891A@IA1PR12MB6044.namprd12.prod.outlook.com>
On Sun, Dec 17, 2023 at 5:54 PM Jonathan Abrams
<jonathansabrams@outlook.com> wrote:
> I am trying to install git-2.43.0 from source on macOS Big Sur 11.7.10. I have Xcode 13.2.1 installed. I have read https://github.com/git/git/blob/master/INSTALL.
>
> The command that will not complete successfully is sudo make all doc. The Terminal output before the error is as follows.
What steps did you take to build the project? Did you simply use `make
all` alone, or did you run `configure` first? Generally speaking,
running `configure` should be unnecessary on most platforms.
> Undefined symbols for architecture x86_64:
> "_libiconv", referenced from:
> _precompose_utf8_readdir in libgit.a(precompose_utf8.o)
> _reencode_string_iconv in libgit.a(utf8.o)
It looks like it's not linking against `libiconv`. Aside from
answering the above question (and reporting the contents of
"config.mak.autogen" if you did run `configure`), you can also run
make as:
make V=1 all
to see the actual command it's running at the point of failure. If you
paste that command, then we can find out whether or not it's trying to
link against `libiconv`.
^ permalink raw reply
* Why is `revert` undocumented in interactive rebase todo help?
From: Michael Lohmann @ 2023-12-18 6:53 UTC (permalink / raw)
To: git; +Cc: Michael Lohmann, Johannes Schindelin
Hi!
I wanted to align rebase and revert/cherry-pick a bit more (for the
latter I am currently finishing my patch for --show-current-patch and
then looked into possibly implementing --edit-todo). To avoid code
duplication I wanted to reuse the existing interactive-rebase code as
much as possible and ended up at the todo script parsing in the
sequencer. I was a bit surprised to find that the file could already
handle the command `revert`, even though it isn't documented in
`append_todo_help` of rebase-interactive.c - is that by choice or just
missing documentation?
Whenever I wanted to achieve this I used `break` and then manually did
the revert, which obviously works fine, but it is much nicer to put the
command in the todo file... (Now that I think about it I could also have
done it with `exec`, but that is also not the nicest solution :D ). The
only other command not mentioned is `noop` which is obviously not too
useful apart from distinguishing an empty list and aborting, so I
totally understand it missing.
Yes - in contrast to all the other options it does not have a single
char notation (and 'r' is obviously already taken und 'u' for undo as
well or 't' for the last letter), but why not show it in the list
without it? Or maybe add 'v' for "reVert"?
Cheers
Michael
P.S.: @Johannes Schindelin I saw your work of making the todo files in
the sequencer more reusable and the many reworks/improvements, so I
added you in cc - I hope that was alright (otherwise I'll buy you a
Kölsch as an apology ;) )...
^ permalink raw reply
* Re: [PATCH] doc: format.notes specify a ref under refs/notes/ hierarchy
From: Patrick Steinhardt @ 2023-12-18 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <xmqqjzpfje33.fsf_-_@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4554 bytes --]
On Fri, Dec 15, 2023 at 02:28:00PM -0800, Junio C Hamano wrote:
> There is no 'ref/notes/' hierarchy. '[format] notes = foo' uses notes
> that are found in 'refs/notes/foo'.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> * According to my eyeballing "git grep refs/ Documentation" result,
> this was the only remaining mention of "ref/" in Documentation/
> hierarchy that misspells "refs/".
This made me look for additional instances where we were referring to
"ref/". Turns out it's only a very limited set, see the below diff. Take
the translation changes with a big grain of salt though, and neither am
I sure whether we want to fix up past release notes. Also, the test is
interesting because it would fail even if we didn't pass an invalid atom
to git-for-each-ref(1).
Anyway, the patch you have looks obviously correct to me. I would be
happy to turn the below diff into a proper patch, but also wouldn't mind
to let you roll them into your patch series. Please let me know your
preference.
Patrick
diff --git a/Documentation/RelNotes/2.1.1.txt b/Documentation/RelNotes/2.1.1.txt
index 830fc3cc6d..d46e142119 100644
--- a/Documentation/RelNotes/2.1.1.txt
+++ b/Documentation/RelNotes/2.1.1.txt
@@ -29,7 +29,7 @@ Git v2.1.1 Release Notes
* "git add x" where x that used to be a directory has become a
symbolic link to a directory misbehaved.
- * The prompt script checked $GIT_DIR/ref/stash file to see if there
+ * The prompt script checked $GIT_DIR/refs/stash file to see if there
is a stash, which was a no-no.
* "git checkout -m" did not switch to another branch while carrying
diff --git a/Documentation/RelNotes/2.2.0.txt b/Documentation/RelNotes/2.2.0.txt
index e98ecbcff6..806908ddb2 100644
--- a/Documentation/RelNotes/2.2.0.txt
+++ b/Documentation/RelNotes/2.2.0.txt
@@ -205,7 +205,7 @@ notes for details).
* "git add x" where x used to be a directory and is now a
symbolic link to a directory misbehaved.
- * The prompt script checked the $GIT_DIR/ref/stash file to see if there
+ * The prompt script checked the $GIT_DIR/refs/stash file to see if there
is a stash, which was a no-no.
* Pack-protocol documentation had a minor typo.
diff --git a/po/fr.po b/po/fr.po
index ee2e610ef1..744550b056 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -19773,7 +19773,7 @@ msgid ""
"Neither worked, so we gave up. You must fully qualify the ref."
msgstr ""
"La destination que vous avez fournie n'est pas un nom de référence complète\n"
-"(c'est-à-dire commençant par \"ref/\"). Essai d'approximation par :\n"
+"(c'est-à-dire commençant par \"refs/\"). Essai d'approximation par :\n"
"\n"
"- Recherche d'une référence qui correspond à '%s' sur le serveur distant.\n"
"- Vérification si la <source> en cours de poussée ('%s')\n"
diff --git a/po/zh_CN.po b/po/zh_CN.po
index 86402725b2..eb47e8f9b7 100644
--- a/po/zh_CN.po
+++ b/po/zh_CN.po
@@ -13224,8 +13224,8 @@ msgid ""
msgid_plural ""
"Note: Some branches outside the refs/remotes/ hierarchy were not removed;\n"
"to delete them, use:"
-msgstr[0] "注意：ref/remotes 层级之外的一个分支未被移除。要删除它，使用："
-msgstr[1] "注意：ref/remotes 层级之外的一些分支未被移除。要删除它们，使用："
+msgstr[0] "注意：refs/remotes 层级之外的一个分支未被移除。要删除它，使用："
+msgstr[1] "注意：refs/remotes 层级之外的一些分支未被移除。要删除它们，使用："
#: builtin/remote.c
#, c-format
diff --git a/po/zh_TW.po b/po/zh_TW.po
index f777a0596f..b2a79cdd93 100644
--- a/po/zh_TW.po
+++ b/po/zh_TW.po
@@ -13109,7 +13109,7 @@ msgid ""
msgid_plural ""
"Note: Some branches outside the refs/remotes/ hierarchy were not removed;\n"
"to delete them, use:"
-msgstr[0] "注意：ref/remotes 層級之外的一個分支未被移除。要刪除它，使用："
+msgstr[0] "注意：refs/remotes 層級之外的一個分支未被移除。要刪除它，使用："
#: builtin/remote.c
#, c-format
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e2281259..e68f7bec8e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -841,7 +841,7 @@ test_expect_success 'err on bad describe atom arg' '
EOF
test_must_fail git for-each-ref \
--format="%(describe:tags,qux=1,abbrev=14)" \
- ref/heads/master 2>actual &&
+ refs/heads/master 2>actual &&
test_cmp expect actual
)
'
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 0/2] fix fetch atomic error message
From: Patrick Steinhardt @ 2023-12-18 8:14 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <cover.1702821462.git.zhiyou.jx@alibaba-inc.com>
[-- Attachment #1: Type: text/plain, Size: 242 bytes --]
On Sun, Dec 17, 2023 at 10:11:32PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> # Changes since v2:
>
> Changed the patches with help from Patrick.
>
Thanks, this version looks good to me!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Patrick Steinhardt @ 2023-12-18 8:24 UTC (permalink / raw)
To: Andy Koppe; +Cc: Ramsay Jones, Junio C Hamano, git
In-Reply-To: <132a3daf-23fa-4575-a77f-bdf0a96fb5d8@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
On Sat, Dec 16, 2023 at 10:20:09AM +0000, Andy Koppe wrote:
> On 15/12/2023 22:44, Ramsay Jones wrote:
> > On 15/12/2023 21:21, Junio C Hamano wrote:
>
> > > If somebody is reading FETCH_HEAD and acting on its contents (rather
> > > than merely consuming it as a ref of the first object), perhaps
> > > feeding it to "git fmt-merge-msg", they will be broken by such a
> > > change (indeed, our own "git pull" will be broken by the change to
> > > "git fetch", and the second bullet point above is about fixing the
> > > exact fallout from it), but I am not sure if that is a use case worth
> > > worrying about.
> >
> > Yes, I was going to suggest exactly this, after Patrick pointed out
> > that there were only two 'special psuedo-refs' (I had a vague feeling
> > there were some more than that) FETCH_HEAD and MERGE_HEAD.
>
> According to the pseudoref entry of gitglossary, CHERRY_PICK_HEAD also
> stores additional data (which would imply that REVERT_HEAD does too).
> Looking at CHERRY_PICK_HEAD during a pick though, I only see a single hash,
> even when picking multiple commits.
Both CHERRY_PICK_HEAD and REVERT_HEAD are only ever updated via the refs
API, so neither of them ever contains anything other than a normal ref.
I guess we should update the glossary accordingly.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Patrick Steinhardt @ 2023-12-18 8:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, git, Andy Koppe
In-Reply-To: <xmqq7clfj7r4.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4024 bytes --]
On Fri, Dec 15, 2023 at 04:44:47PM -0800, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
> > Yes, I was going to suggest exactly this, after Patrick pointed out
> > that there were only two 'special psuedo-refs' (I had a vague feeling
> > there were some more than that) FETCH_HEAD and MERGE_HEAD.
I don't think there are more special refs than those two. Andy pointed
out CHERRY_PICK_HEAD and REVERT_HEAD, but both of them actually get
accessed via the ref backend exclusively and thus cannot be special in
any way. Also, the test suite of Git passes with only those two refs
marked as special refs with the reftable backend, which is another good
indicator that I didn't miss anything here because we definitely can't
store special information in the reftable backend.
It's of course still possible that our test suite has a blind spot and
that I missed any special refs. If so, I would love to hear about them.
> Glad to see that I am not alone. We should be able to treat
> MERGE_HEAD similarly. It is used to communicate the list of "other
> parents" from "git merge" that stops in the middle (either for merge
> conflict, or in response to the "--no-commit" command line option)
> to "git commit" that concludes such an unfinished merge. Many
> commands merely use the presence of MERGE_HEAD as a sign that a
> merge is in progress (e.g. "git status"), which would not break if
> we just started to record the first parent in a pseudoref MERGE_HEAD
> and wrote the other octopus parents elsewhere, but some commands do
> need all these parents from MERGE_HEAD (e.g. "git blame" that
> synthesizes a fake starting commit out of the working tree state).
I would certainly love to drop the "specialness" of both FETCH_HEAD and
MERGE_HEAD, but I am a bit pessimistic about whether we really can. The
format of those refs has been around for quite a long time already, and
I do expect that there is tooling out there that parses those files.
I would claim that it's especially likely that FETCH_HEAD is getting
parsed by external tools. Historically, there has not been a way to
really figure out which refs have been updated in git-fetch(1). So any
scripts that perform a fetch and want to learn about what was updated
would very likely resort to parsing FETCH_HEAD. This has changed a bit
with the introduction of the machine-parsable interface of git-fetch(1),
but it has only been introduced rather recently with Git v2.42.
> If we cannot get rid of all "special refs" anyway, however, I think
> there is little that we can gain from doing such "make FETCH_HEAD
> and MERGE_HEAD into a single-object pseudoref, and write other info
> in separate files" exercise. We can treat the current FETCH_HEAD
> and MERGE_HEAD as "file that is not and is more than a ref", which
> is what the current code is doing anyway, which means we would
> declare that they have to stay to be files under $GIT_DIR/ and will
> be accessed via the filesystem access.
I'd like for it to be otherwise, but I think this is the only sensible
thing to do. I think it was a mistake to introduce those special refs
like this and treat them almost like a real ref, but that's always easy
to say in hindsight.
> At that point, calling them "special ref" might even be more
> misleading than its worth and we may be better off to admit that they
> are not even refs but a datafile some commands can use to obtain input
> from, but the phrase we use to refer to them, be it "special ref" or
> some random datafile, does not make a fundamental change on anything.
Well, the problem is that these do indeed behave like a ref for most of
the part: you can ask for them via git-rev-parse(1) and we'll resolve
them just fine, even though we only ever return the first object ID. So
even though I'm not a huge fan of calling them "special ref", I think we
should at least highlight the reflike-nature in whatever we want to call
them.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] git.txt: HEAD is not that special
From: Patrick Steinhardt @ 2023-12-18 8:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20231215203245.3622299-2-gitster@pobox.com>
[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]
On Fri, Dec 15, 2023 at 12:32:41PM -0800, Junio C Hamano wrote:
> The introductory text in "git help git" that describes HEAD called
> it "a special ref". It is special compared to the more regular refs
> like refs/heads/master and refs/tags/v1.0.0, but not that special,
> unlike truly special ones like FETCH_HEAD.
>
> Rewrite a few sentences to also introduce the distinction between a
> regular ref that contain the object name and a symbolic ref that
> contain the name of another ref. Update the description of HEAD
> that point at the current branch to use the more correct term, a
> "symbolic ref".
>
> This was found as part of auditing the documentation and in-code
> comments for uses of "special ref" that refer merely a "pseudo ref".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/git.txt | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2535a30194..880cdc5d7f 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1025,10 +1025,11 @@ When first created, objects are stored in individual files, but for
> efficiency may later be compressed together into "pack files".
>
> Named pointers called refs mark interesting points in history. A ref
> -may contain the SHA-1 name of an object or the name of another ref. Refs
> -with names beginning `ref/head/` contain the SHA-1 name of the most
> +may contain the SHA-1 name of an object or the name of another ref (the
> +latter is called a "symbolic ref").
On a tangent: While we have a name for symbolic refs, do we also have a
name for non-symbolic refs? I often use the term "direct ref" to clearly
distinguish them from symbolic refs, but it's of course not defined in
our glossary.
> +Refs with names beginning `ref/head/` contain the SHA-1 name of the most
> recent commit (or "head") of a branch under development. SHA-1 names of
> -tags of interest are stored under `ref/tags/`. A special ref named
> +tags of interest are stored under `ref/tags/`. A symbolic ref named
> `HEAD` contains the name of the currently checked-out branch.
I was briefly wondering whether we also want to replace SHA-1 with
"object hash" while at it, but it's certainly out of the scope of this
patch series.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] make room for "special ref"
From: Patrick Steinhardt @ 2023-12-18 8:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20231215203245.3622299-1-gitster@pobox.com>
[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]
On Fri, Dec 15, 2023 at 12:32:40PM -0800, Junio C Hamano wrote:
> Patrick's reftable work is progressing nicely and wants to establish
> "special ref" as a phrase with some defined meaning that is somewhat
> different from a mere "pseudo ref".
>
> A pseudo ref is merely a normal ref with a funny naming convention,
> i.e., being outside the refs/ hierarchy and has names with all
> uppercase letters (or an underscore). But there truly are refs that
> are more than that. For example, FETCH_HEAD currently stores not
> just a single object name, but can and is used to store multiple
> object names, each with annotations to record where they came from.
> There indeed may be a need to introduce a new term to refer to such
> "special refs".
>
> Existing documentation, however, uses "special ref" to refer to
> pseudo refs without any "special" property, like FETCH_HEAD does.
>
> This series merely corrects such existing uses of the word, to make
> room for Patrick's series to introduce (and formally define in the
> glossary) "special refs".
Thanks for helping out with this effort and kicking off the discussion,
I highly appreciate it!
Patrick
> Junio C Hamano (5):
> git.txt: HEAD is not that special
> git-bisect.txt: BISECT_HEAD is not that special
> refs.h: HEAD is not that special
> docs: AUTO_MERGE is not that special
> docs: MERGE_AUTOSTASH is not that special
>
> Documentation/git-bisect.txt | 2 +-
> Documentation/git-diff.txt | 2 +-
> Documentation/git-merge.txt | 2 +-
> Documentation/git.txt | 7 ++++---
> Documentation/merge-options.txt | 2 +-
> Documentation/user-manual.txt | 2 +-
> refs.h | 2 +-
> 7 files changed, 10 insertions(+), 9 deletions(-)
>
> --
> 2.43.0-76-g1a87c842ec
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] commit-graph: fix memory leak when not writing graph
From: Patrick Steinhardt @ 2023-12-18 10:02 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
When `write_commit_graph()` bails out writing a split commit-graph early
then it may happen that we have already gathered the set of existing
commit-graph file names without yet determining the new merged set of
files. This can result in a memory leak though because we only clear the
preimage of files when we have collected the postimage.
Fix this issue by dropping the condition altogether so that we always
try to free both preimage and postimage filenames. As the context
structure is zero-initialized this simplification is safe to do.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
commit-graph.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index acac9bf6e1..afe0177ab2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2617,19 +2617,16 @@ int write_commit_graph(struct object_directory *odb,
oid_array_clear(&ctx->oids);
clear_topo_level_slab(&topo_levels);
- if (ctx->commit_graph_filenames_after) {
- for (i = 0; i < ctx->num_commit_graphs_after; i++) {
- free(ctx->commit_graph_filenames_after[i]);
- free(ctx->commit_graph_hash_after[i]);
- }
-
- for (i = 0; i < ctx->num_commit_graphs_before; i++)
- free(ctx->commit_graph_filenames_before[i]);
+ for (i = 0; i < ctx->num_commit_graphs_before; i++)
+ free(ctx->commit_graph_filenames_before[i]);
+ free(ctx->commit_graph_filenames_before);
- free(ctx->commit_graph_filenames_after);
- free(ctx->commit_graph_filenames_before);
- free(ctx->commit_graph_hash_after);
+ for (i = 0; i < ctx->num_commit_graphs_after; i++) {
+ free(ctx->commit_graph_filenames_after[i]);
+ free(ctx->commit_graph_hash_after[i]);
}
+ free(ctx->commit_graph_filenames_after);
+ free(ctx->commit_graph_hash_after);
free(ctx);
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: Why is `revert` undocumented in interactive rebase todo help?
From: Johannes Schindelin @ 2023-12-18 10:53 UTC (permalink / raw)
To: Michael Lohmann; +Cc: git, Michael Lohmann
In-Reply-To: <20231218065313.55725-1-mi.al.lohmann@gmail.com>
Hi Michael,
On Mon, 18 Dec 2023, Michael Lohmann wrote:
> I wanted to align rebase and revert/cherry-pick a bit more (for the
> latter I am currently finishing my patch for --show-current-patch and
> then looked into possibly implementing --edit-todo). To avoid code
> duplication I wanted to reuse the existing interactive-rebase code as
> much as possible and ended up at the todo script parsing in the
> sequencer. I was a bit surprised to find that the file could already
> handle the command `revert`, even though it isn't documented in
> `append_todo_help` of rebase-interactive.c - is that by choice or just
> missing documentation?
The reason that it is not documented, and that it has no single-letter
"short command", is that it is more of a historic accident than design
that interactive rebases support the "revert" command: In 004fefa754a4
(sequencer: completely revamp the "todo" script parsing, 2016-10-21), I
revamped sequencer's parsing of the "todo script", in preparation for
teaching it the trick to parse full-blown todo scripts of interactive
rebases in addition to parsing the (hitherto quite limited) `cherry-pick`
and `revert` "scripts", a trick that was completed with cca3be6ea15b
(Merge branch 'js/prepare-sequencer', 2016-10-27). These days, `git rebase
--interactive` uses that code to parse todo scripts.
Naturally, to be able to continue parsing the "revert scripts", the
`revert` command needed to be supported, and I never thought of disabling
it specifically for interactive rebases.
> Whenever I wanted to achieve this I used `break` and then manually did
> the revert, which obviously works fine, but it is much nicer to put the
> command in the todo file... (Now that I think about it I could also have
> done it with `exec`, but that is also not the nicest solution :D ).
Right. I often find myself adding commands like this one:
x git revert -n <reverse-fixup> && git commit --amend --no-edit
to amend a commit with a reversal of another commit, most prominently when
I squashed a fixup into another commit than I had originally intended and
now need to fix that.
> The only other command not mentioned is `noop` which is obviously not
> too useful apart from distinguishing an empty list and aborting, so I
> totally understand it missing.
Correct, that one is intentionally not described, for the reasons you
described.
> Yes - in contrast to all the other options it does not have a single
> char notation (and 'r' is obviously already taken und 'u' for undo as
> well or 't' for the last letter), but why not show it in the list
> without it? Or maybe add 'v' for "reVert"?
Sure ;-)
Ciao,
Johannes
^ permalink raw reply
* Re: Question regarding Git updater
From: Johannes Schindelin @ 2023-12-18 11:21 UTC (permalink / raw)
To: Andreas Scholz; +Cc: git
In-Reply-To: <CAHDWvZyHDbjOnnCYCkfMY+HPWobrcgP6c1kkWFrRgWV4fHED=w@mail.gmail.com>
Hi Andreas,
On Thu, 14 Dec 2023, Andreas Scholz wrote:
> I hope you can help me with answering my question regarding the update
> mechanism for Git after it has been installed.
Assuming that you mean Git for Windows and its updater that you can enable
by checking the "Check daily for Git for Windows updates" option on the
Components page, I will provide answers below (if you are not talking
about Git for Windows, I apologize, and ask for clarification):
> 1) Does the updater autonomously figure out if there is a newer
> version than the current one that is installed?
Git for Windows' updater is backed by a Unix shell script:
https://github.com/git-for-windows/build-extra/blob/HEAD/git-extra/git-update-git-for-windows
When configured to run regularly via a scheduled task, it will be called
with the options `--quiet --gui`. It starts by determining the latest tag:
https://github.com/git-for-windows/build-extra/blob/15b05c2399f152783d1fe9f167692dd5cd8ae1e1/git-extra/git-update-git-for-windows#L222
which downloads https://gitforwindows.org/latest-tag.txt, a file that is
hosted on GitHub Pages and that is updated as part of every Git for
Windows release.
The script then continues by determining the local version:
https://github.com/git-for-windows/build-extra/blob/15b05c2399f152783d1fe9f167692dd5cd8ae1e1/git-extra/git-update-git-for-windows#L248
and comparing both versions:
https://github.com/git-for-windows/build-extra/blob/15b05c2399f152783d1fe9f167692dd5cd8ae1e1/git-extra/git-update-git-for-windows#L257-L262
exiting with success if they match.
Most notably, it does _not_ verify that the remote version is newer,
meaning: If you build and install a custom Git version that reports a
version number, say, 100.100.100, the updater will still propose to update
from that, even if the current version is v2.43.0.
> 2) Or does the updater only ask, when the user actively uses a command
> to ask Git to check for a newer version?
Users are welcome to run `git update-git-for-windows` manually. If
aforementioned checkbox was checked during installation, this won't be
necessary, strictly speaking.
> 3) In both cases, what information about the user/system is sent with
> the request? Is this information stored on a server or database etc.?
The information that is sent is the IP address and the HTTP headers sent
by `curl` in
https://github.com/git-for-windows/build-extra/blob/15b05c2399f152783d1fe9f167692dd5cd8ae1e1/git-extra/git-update-git-for-windows#L120-L125
i.e. a User-Agent (`curl` does not seem to include the current OS there),
but not the current Git for Windows version (an information that, if
available, would actually help me perform my role of Git for Windows
maintainer a lot better). Since the request goes to GitHub Pages, which
does not store any information, all of that information vanishes right
after the HTTP response is sent.
Ciao,
Johannes
^ permalink raw reply
* [PATCH 0/1] revert/cherry-pick: add --show-current-patch option
From: Michael Lohmann @ 2023-12-18 12:10 UTC (permalink / raw)
To: git; +Cc: Michael Lohmann, Elijah Newren
Hi,
I am a lead developer of a small team and quite often I have to
cherry-pick commits (and sometimes also revert them). When
cherry-picking multiple commits at once and there is a merge conflict it
sometimes can be hard to understand what the current patch is trying to
do in order to resolve the conflict properly. With `rebase` there is
`--show-current-patch` and since that is quite helpful I would suggest
to also add this flag also to `cherry-pick` and `revert`.
Since this is my first contribution to git I am not exactly sure where
the best place for this functionality is. From my initial understanding
there are two places where to put the actual invocation of the `show`:
- Duplicate the code (with the needed adaptations) of builtin/rebase.c
in builtin/revert.c
- Create a central function that shows the respective `*_HEAD` depending
on the current `action`.
In this first draft I went with the second option, since I felt that it
reduces code duplication and the sequencer already has the action enum
with exactly those three cases. On the other hand I don’t really have a
good understanding of the role that this `sequencer` should play and if
this adds additional coupling that is unwanted. My current impression
is, that this would be the right place, since this looks to be the core
of the commands where a user can apply a sequence of commits and in my
opinion even if additional actions would be added, they could also fail
and so it would be good to add the `--show-current-patch` option to that
one as well.
Side note: my only C(++) experience was ~10 years ago and only for a
single university course, so my perspective is much more from a general
architecture point of view than based on any C experience, let alone in
this code base and so I would be very grateful for criticism!
Side note: The check for the `REBASE_HEAD` would not be necessary, since
that is already taken care of in the builtin/rebase.c before.
Nevertheless I opted for this check, because I would much rather require
the same preconditions no matter from where I call this function. The
whole argument parsing / option struct are very different between rebase
and revert. Maybe it would make sense to align them a bit further?
Initial observations: `rebase_options->type` is functionally similar to
`replay_opts->action` (as in "what general action am I performing? -
interactive rebase / cherry-pick / revert / ...") whereas
`rebase_options->action` is not part of the `replay_opts` struct at all.
Instead the role is taken over in builtin/revert.c by `int cmd = 0;`.
I am preparing a patch converting this to an enum, so that there are
no random chars that have to be kept in sync manually in different
places, or is that a design decision?
I looked through the mailing list archive and did not find anything
related on this topic. The only slightly related thread I could find was
in [1] by Elijah Newren and that one was talking about a separate
possible feature and how to get certain information if CHERRY_PICK_HEAD
and REVERT_HEAD were to be replaced by a different construct. I hope I
did not miss something...
Cheers
Michael
[1]:
https://lore.kernel.org/git/CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com
Michael Lohmann (1):
revert/cherry-pick: add --show-current-patch option
Documentation/git-cherry-pick.txt | 2 +-
Documentation/git-revert.txt | 2 +-
Documentation/sequencer.txt | 5 +++++
builtin/rebase.c | 7 ++----
builtin/revert.c | 9 ++++++--
contrib/completion/git-completion.bash | 2 +-
sequencer.c | 24 +++++++++++++++++++++
sequencer.h | 2 ++
t/t3507-cherry-pick-conflict.sh | 30 ++++++++++++++++++++++++++
9 files changed, 73 insertions(+), 10 deletions(-)
--
2.43.0.77.gff6ea8bb74
^ 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