* [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"
From: Josh Triplett @ 2024-02-26 4:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
The scissors line before the diff in a verbose commit, or above all the
comments when using --cleanup=scissors, has the following two lines of
explanation after it:
Do not modify or remove the line above.
Everything below it will be ignored.
This is useful advice for new users, but potentially redundant for
experienced users, who might instead appreciate seeing two more lines of
information in their editor.
Add advice.scissors to suppress that explanation.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
Documentation/config/advice.txt | 5 +++++
advice.c | 1 +
advice.h | 1 +
wt-status.c | 3 ++-
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2..33ab688b6c 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -104,6 +104,11 @@ advice.*::
rmHints::
In case of failure in the output of linkgit:git-rm[1],
show directions on how to proceed from the current state.
+ scissors::
+ Advice shown by linkgit:git-commit[1] in the commit message
+ opened in an editor, after a scissors line (containing >8),
+ saying not to remove the line and that everything after the line
+ will be ignored.
sequencerInUse::
Advice shown when a sequencer command is already in progress.
skippedCherryPicks::
diff --git a/advice.c b/advice.c
index 6e9098ff08..0588012562 100644
--- a/advice.c
+++ b/advice.c
@@ -71,6 +71,7 @@ static struct {
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
[ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SCISSORS] = { "scissors" },
[ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
[ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
[ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38..9725aa4199 100644
--- a/advice.h
+++ b/advice.h
@@ -39,6 +39,7 @@ enum advice_type {
ADVICE_RESET_NO_REFRESH_WARNING,
ADVICE_RESOLVE_CONFLICT,
ADVICE_RM_HINTS,
+ ADVICE_SCISSORS,
ADVICE_SEQUENCER_IN_USE,
ADVICE_SET_UPSTREAM_FAILURE,
ADVICE_SKIPPED_CHERRY_PICKS,
diff --git a/wt-status.c b/wt-status.c
index 459d399baa..19d4986351 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1104,7 +1104,8 @@ void wt_status_append_cut_line(struct strbuf *buf)
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
- strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+ if (advice_enabled(ADVICE_SCISSORS))
+ strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
void wt_status_add_cut_line(FILE *fp)
--
2.43.0
^ permalink raw reply related
* [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Josh Triplett @ 2024-02-26 4:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
`git commit --cleanup=scissors -v` currently prints two scissors lines:
one at the start of the comment lines, and the other right before the
diff. This is redundant, and pushes the diff further down in the user's
editor than it needs to be.
Pass the cleanup mode into wt_status, so that wt_status_print can avoid
printing the extra scissors if already printed.
This moves the enum commit_msg_cleanup_mode from sequencer.h to
wt-status.h to allow wt_status to use the type. sequencer.h already
includes wt-status.h, so this doesn't affect anything else.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
builtin/commit.c | 2 ++
sequencer.h | 7 -------
wt-status.c | 6 ++++--
wt-status.h | 8 ++++++++
4 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6d1fa71676..6b2b412932 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -888,6 +888,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
*/
s->hints = 0;
+ s->cleanup_mode = cleanup_mode;
+
if (clean_message_contents)
strbuf_stripspace(&sb, '\0');
diff --git a/sequencer.h b/sequencer.h
index dcef7bb99c..9f818e96f0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -22,13 +22,6 @@ enum replay_action {
REPLAY_INTERACTIVE_REBASE
};
-enum commit_msg_cleanup_mode {
- COMMIT_MSG_CLEANUP_SPACE,
- COMMIT_MSG_CLEANUP_NONE,
- COMMIT_MSG_CLEANUP_SCISSORS,
- COMMIT_MSG_CLEANUP_ALL
-};
-
struct replay_opts {
enum replay_action action;
diff --git a/wt-status.c b/wt-status.c
index b5a29083df..459d399baa 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
* file (and even the "auto" setting won't work, since it
* will have checked isatty on stdout). But we then do want
* to insert the scissor line here to reliably remove the
- * diff before committing.
+ * diff before committing, if we didn't already include one
+ * before.
*/
if (s->fp != stdout) {
rev.diffopt.use_color = 0;
- wt_status_add_cut_line(s->fp);
+ if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS)
+ wt_status_add_cut_line(s->fp);
}
if (s->verbose > 1 && s->committable) {
/* print_updated() printed a header, so do we */
diff --git a/wt-status.h b/wt-status.h
index 819dcad723..5ede705e93 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -22,6 +22,13 @@ enum color_wt_status {
WT_STATUS_MAXSLOT
};
+enum commit_msg_cleanup_mode {
+ COMMIT_MSG_CLEANUP_SPACE,
+ COMMIT_MSG_CLEANUP_NONE,
+ COMMIT_MSG_CLEANUP_SCISSORS,
+ COMMIT_MSG_CLEANUP_ALL
+};
+
enum untracked_status_type {
SHOW_NO_UNTRACKED_FILES,
SHOW_NORMAL_UNTRACKED_FILES,
@@ -130,6 +137,7 @@ struct wt_status {
int rename_score;
int rename_limit;
enum wt_status_format status_format;
+ enum commit_msg_cleanup_mode cleanup_mode;
struct wt_status_state state;
struct object_id oid_commit; /* when not Initial */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v5 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Junio C Hamano @ 2024-02-26 4:35 UTC (permalink / raw)
To: Philippe Blain
Cc: git, Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Michael Lohmann
In-Reply-To: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-2-af1ef2d9e44d@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
> + for (i = 0; i < ARRAY_SIZE(other_head); i++)
> + if (!read_ref_full(other_head[i],
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + oid, NULL)) {
> + if (is_null_oid(oid))
> + die(_("%s is a symbolic ref?"), other_head[i]);
> + return other_head[i];
> + }
> +
> + die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
> +}
Just a minor nit, but reacting to recent "passive-aggressive"
message change in another thread, perhaps we should stop asking a
rhetorical question like the new message and instead state what we
detected and what we consider is an error condition as a fact in
them.
The last die() in the above helper function used to be such a
rhetorical question "--merge without HEAD?" but now it reads much
better. The one about symbolic ref is new in this series, and we
can avoid making it rhetorical from the get go. Perhaps "%s exists
but it is a symbolic ref" or something?
^ permalink raw reply
* CR position changed in exported patch file subject section
From: Chen, Boyang @ 2024-02-26 5:45 UTC (permalink / raw)
To: git@vger.kernel.org
[AMD Official Use Only - General]
Hi guys,
Recently I faced a question that the CR position in the patch is not in line with the git log.
Below are the steps to reproduce.
touch aaa
git add aaa
git commit -m "Add a file to test, make sure that the message is a bit long but in a single line"
D:\Source\CustomerRepoTest\Platform1>git log -3
commit 0c9f8555c55c73fd4e5392c8f8516c389f362d17 (HEAD -> test)
Author: Boyang Chen
Date: Mon Feb 26 11:16:00 2024 +0800
Add a file to test, make make sure that the message is a bit long but in a single line
We can confirm that the commit message is in a single line in the output of git log command(pls refer to above output).
And use below command to generate a patch file.
git format-patch -3 --stdout > exported_3.patch
We can observe that the commit message's CR position is changed in the exported patch, the subject section is split to two lines(pls refer to below output).
From 0c9f8555c55c73fd4e5392c8f8516c389f362d17 Mon Sep 17 00:00:00 2001
From: Boyang Chen
Date: Mon, 26 Feb 2024 11:16:00 +0800
Subject: [PATCH 3/3] Add a file to test, make make sure that the message is a
bit long but in a single line
---
aaa | 0
1 file changed, 0 insertions(+), 0 deletions(-)
Actually, we want to keep the commit message format(subject section) in the patch same as the git log output.
I have tried most of arguments of git format-patch and even upgrade the git version.
But no good result is observed.
Could you please help to look at this question? Thanks a lot!
BR
Boyang
^ permalink raw reply
* RE: CR position changed in exported patch file subject section
From: Chen, Boyang @ 2024-02-26 5:53 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <IA0PR12MB822711B89738EDA0E2F25150EF5A2@IA0PR12MB8227.namprd12.prod.outlook.com>
[AMD Official Use Only - General]
Hi guys,
Pls ignore the first email as there is some format mistake in it and pick this one.
Recently I faced a question that the CR position in the patch is not in line with the git log.
Below are the steps to reproduce.
touch aaa
git add aaa
git commit -m "Add a file to test, make sure that the message is a bit long but in a single line"
D:\Source\CustomerRepoTest\Platform1>git log -3 commit 0c9f8555c55c73fd4e5392c8f8516c389f362d17 (HEAD -> test)
Author: Boyang Chen
Date: Mon Feb 26 11:16:00 2024 +0800
Add a file to test, make make sure that the message is a bit long but in a single line
We can confirm that the commit message is in a single line in the output of git log command(pls refer to above output).
And use below command to generate a patch file.
git format-patch -3 --stdout > exported_3.patch
We can observe that the commit message's CR position is changed in the exported patch, the subject section is split to two lines(pls refer to below output).
From 0c9f8555c55c73fd4e5392c8f8516c389f362d17 Mon Sep 17 00:00:00 2001
From: Boyang Chen
Date: Mon, 26 Feb 2024 11:16:00 +0800
Subject: [PATCH 3/3] Add a file to test, make make sure that the message is a
bit long but in a single line
---
aaa | 0
1 file changed, 0 insertions(+), 0 deletions(-)
Actually, we want to keep the commit message format(subject section) in the patch same as the git log output.
I have tried most of arguments of git format-patch and even upgrade the git version.
But no good result is observed.
Could you please help to look at this question? Thanks a lot!
BR
Boyang
^ permalink raw reply
* Re: CR position changed in exported patch file subject section
From: Jeff King @ 2024-02-26 7:02 UTC (permalink / raw)
To: Chen, Boyang; +Cc: git@vger.kernel.org
In-Reply-To: <IA0PR12MB822712F1B3E5205711493D55EF5A2@IA0PR12MB8227.namprd12.prod.outlook.com>
On Mon, Feb 26, 2024 at 05:53:19AM +0000, Chen, Boyang wrote:
> D:\Source\CustomerRepoTest\Platform1>git log -3 commit 0c9f8555c55c73fd4e5392c8f8516c389f362d17 (HEAD -> test)
> Author: Boyang Chen
> Date: Mon Feb 26 11:16:00 2024 +0800
>
> Add a file to test, make make sure that the message is a bit long but in a single line
>
> We can confirm that the commit message is in a single line in the output of git log command(pls refer to above output).
>
> And use below command to generate a patch file.
> git format-patch -3 --stdout > exported_3.patch
>
> We can observe that the commit message's CR position is changed in the
> exported patch, the subject section is split to two lines(pls refer to
> below output).
This is expected. The format-patch command is generating an email, and
rfc2822 says:
Each line of characters MUST be no more than 998 characters, and
SHOULD be no more than 78 characters, excluding the CRLF.
But we can make the subject arbitrarily long by using header
continuation; the line after the "Subject:" should start with
whitespace, which indicates to a parser that it is a continuation of the
previous header.
You don't show that here:
> From 0c9f8555c55c73fd4e5392c8f8516c389f362d17 Mon Sep 17 00:00:00 2001
> From: Boyang Chen
> Date: Mon, 26 Feb 2024 11:16:00 +0800
> Subject: [PATCH 3/3] Add a file to test, make make sure that the message is a
> bit long but in a single line
but I'm not sure if it's really missing, or if the whitespace got munged
as you sent it. Assuming it is, then everything is working as designed.
That said, I have sometimes been annoyed at this myself, because I want
to process the mails with tools that are quite capable of handling long
lines (e.g., mutt). And doing hacky processing with perl, etc, becomes
harder because you have to actually parse the mail correctly rather than
just grepping for "^Subject:". ;)
So I have wondered if it would be useful to have a --no-wrap-email
option. Or perhaps the existing --no-encode-email-headers should be used
as a hint that the user prefers easy-to-parse output over strict rfc
compliance.
-Peff
^ permalink raw reply
* Bug: diff --no-index with cachetextconv crashes
From: Paweł Dominiak @ 2024-02-26 7:03 UTC (permalink / raw)
To: git
Hey!
That's my first bug report for git and my first email to a mailing
list in general, I hope for understanding :)
[Steps to reproduce your issue]
Global .gitattributes:
*.txt diff=test
Global .gitconfig:
[diff "test"]
textconv = cat
cachetextconv = true
Called command:
git --no-pager diff --no-index foo.txt bar.txt
[Expected behavior]
diff --git a/foo.txt b/bar.txt
index f6a4b70..2b24d27 100644
--- a/foo.txt
+++ b/bar.txt
@@ -1 +1 @@
-Foo bar baz
+Foo bar qux
[Actual behavior]
BUG: refs.c:2095: attempting to get main_ref_store outside of repository
The command works as expected if cachetextconv is disabled:
git --no-pager -c diff.test.cachetextconv=false diff --no-index foo.txt bar.txt
[System Info]
git version 2.44.0.windows.1
cpu: x86_64
built from commit: ad0bbfffa543db6979717be96df630d3e5741331
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
not run from a git repository - no hooks to show
Pawel
^ permalink raw reply related
* Re: [PATCH 1/2] mem-pool: add mem_pool_strfmt()
From: Jeff King @ 2024-02-26 7:08 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20240225113947.89357-2-l.s.r@web.de>
On Sun, Feb 25, 2024 at 12:39:44PM +0100, René Scharfe wrote:
> +static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,
> + va_list ap)
> +{
> + struct mp_block *block = pool->mp_block;
> + char *next_free = block ? block->next_free : NULL;
> + size_t available = block ? block->end - block->next_free : 0;
> + va_list cp;
> + int len, len2;
> + char *ret;
> +
> + va_copy(cp, ap);
> + len = vsnprintf(next_free, available, fmt, cp);
> + va_end(cp);
> + if (len < 0)
> + BUG("your vsnprintf is broken (returned %d)", len);
> +
> + ret = mem_pool_alloc(pool, len + 1); /* 1 for NUL */
> +
> + /* Shortcut; relies on mem_pool_alloc() not touching buffer contents. */
> + if (ret == next_free)
> + return ret;
> +
> + len2 = vsnprintf(ret, len + 1, fmt, ap);
> + if (len2 != len)
> + BUG("your vsnprintf is broken (returns inconsistent lengths)");
> + return ret;
> +}
This is pulling heavily from strbuf_vaddf(). This might be a dumb idea,
but... would it be reasonable to instead push a global flag that causes
xmalloc() to use a memory pool instead of the regular heap?
Then you could do something like:
push_mem_pool(pool);
str = xstrfmt("%.*s~%d^%d", ...etc...);
pop_mem_pool(pool);
It's a little more involved at the caller, but it means that it now
works for all allocations, not just this one string helper.
Obviously you'd want it to be a thread-local value to prevent races. But
I still wonder if it could cause havoc when some sub-function makes an
allocation that the caller does not expect.
-Peff
^ permalink raw reply
* Re: Bug: diff --no-index with cachetextconv crashes
From: Jeff King @ 2024-02-26 7:22 UTC (permalink / raw)
To: Paweł Dominiak; +Cc: git
In-Reply-To: <CACeVQwQ4MELjB8nZyeu9QDTtgwhhw0oOsL8BHdm_rxTj1vMy+A@mail.gmail.com>
On Mon, Feb 26, 2024 at 08:03:04AM +0100, Paweł Dominiak wrote:
> That's my first bug report for git and my first email to a mailing
> list in general, I hope for understanding :)
Hi, welcome, and thanks for a clear bug report. :)
> [Steps to reproduce your issue]
>
> Global .gitattributes:
>
> *.txt diff=test
>
> Global .gitconfig:
>
> [diff "test"]
> textconv = cat
> cachetextconv = true
>
> Called command:
>
> git --no-pager diff --no-index foo.txt bar.txt
OK, I would say that this failing is semi-expected. :) The caching
system works using "git notes", which are stored in refs in the
repository. And since you are running "diff --no-index" outside of a
repository, there is nowhere to put them.
And so this BUG call makes sense:
> BUG: refs.c:2095: attempting to get main_ref_store outside of repository
We tried to load notes but there's no ref store at all, and the
low-level ref code caught this.
Of course any time we see a BUG something has gone wrong. What I think
_should_ happen is that we should quietly disable the caching (which,
after all, is just an optimization) and otherwise complete the command.
So we'd probably want something like this:
diff --git a/userdiff.c b/userdiff.c
index e399543823..fce3a31efa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -3,6 +3,7 @@
#include "userdiff.h"
#include "attr.h"
#include "strbuf.h"
+#include "environment.h"
static struct userdiff_driver *drivers;
static int ndrivers;
@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,
if (!driver->textconv)
return NULL;
- if (driver->textconv_want_cache && !driver->textconv_cache) {
+ if (driver->textconv_want_cache && !driver->textconv_cache &&
+ have_git_dir()) {
struct notes_cache *c = xmalloc(sizeof(*c));
struct strbuf name = STRBUF_INIT;
-Peff
^ permalink raw reply related
* [Question] How to parse range-diff output
From: ZheNing Hu @ 2024-02-26 7:25 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Christian Couder, Johannes Schindelin
Hi,
I am currently looking to implement a service that provides a version
range comparison based on git range-diff. I can easily parse out
commit pair headers like "3: 0bf6289 ! 3: a076e88 dev5," but I am
unsure how to parse the details in the subsequent diff patch body.
It is not a standard diff output where one can parse out the filename
from the diff header, It should be called a diff of diffs. We can see
various headers with file names such as "@@ File1 (new)", "## File2
(new) ##", or "@@ File3: function3" in different formats. This is
confusing. How should we correctly parse a range-diff patch, and do
you have any good suggestions?
Thanks for any help.
--
ZheNing Hu
^ permalink raw reply
* Re: Bug: diff --no-index with cachetextconv crashes
From: Paweł Dominiak @ 2024-02-26 7:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240226072248.GC780982@coredump.intra.peff.net>
> OK, I would say that this failing is semi-expected. :) The caching
> system works using "git notes", which are stored in refs in the
> repository. And since you are running "diff --no-index" outside of a
> repository, there is nowhere to put them.
I have not mentioned this specifically, but my goal is a general diff
command, which internally uses text conversions, pager etc. as
configured for git.
It makes sense to cache the textconv results when used in a
repository, but I don't think it should fail when not in one.
I think the default behavior should be to silently skip caching in
such situations but produce a diff otherwise.
> Of course any time we see a BUG something has gone wrong. What I think
> _should_ happen is that we should quietly disable the caching (which,
> after all, is just an optimization) and otherwise complete the command.
In my script I currently disable caching explicitly for all drivers:
keys=$(git config --name-only --get-regexp '^diff\.\w+\.cachetextconv$')
config=(); for key in $keys; do config+=(-c "$key=false"); done
git "${config[@]}" diff --no-index --no-prefix "$@"
But it seems like something git should handle on its own, so that diff
would accommodate use in different circumstances with the same config.
-Pawel
^ permalink raw reply
* RE: CR position changed in exported patch file subject section
From: Chen, Boyang @ 2024-02-26 8:21 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20240226070255.GA780982@coredump.intra.peff.net>
[AMD Official Use Only - General]
Hi Peff,
Thanks for your clarification.
The commit message is just a single line, but the subject section is splinted into two lines as the line characters number is more than 78.
This is in line with the design.
The content you read doesn't have this change, maybe it is caused by the outlook content format.
perhaps the existing --no-encode-email-headers should be used as a hint that the user prefers easy-to-parse output over strict rfc compliance.
==> I have tried this command, the generated patch subject section is still splinted into two lines.
I have wondered if it would be useful to have a --no-wrap-email option.
==> if we add this new option and output the commit message as the way that git log command does, it should be good with usage.
Thanks again!
BR
Boyang
-----Original Message-----
From: Jeff King <peff@peff.net>
Sent: Monday, February 26, 2024 3:03 PM
To: Chen, Boyang <Boyang.Chen@amd.com>
Cc: git@vger.kernel.org
Subject: Re: CR position changed in exported patch file subject section
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On Mon, Feb 26, 2024 at 05:53:19AM +0000, Chen, Boyang wrote:
> D:\Source\CustomerRepoTest\Platform1>git log -3 commit
> 0c9f8555c55c73fd4e5392c8f8516c389f362d17 (HEAD -> test)
> Author: Boyang Chen
> Date: Mon Feb 26 11:16:00 2024 +0800
>
> Add a file to test, make make sure that the message is a bit long
> but in a single line
>
> We can confirm that the commit message is in a single line in the output of git log command(pls refer to above output).
>
> And use below command to generate a patch file.
> git format-patch -3 --stdout > exported_3.patch
>
> We can observe that the commit message's CR position is changed in the
> exported patch, the subject section is split to two lines(pls refer to
> below output).
This is expected. The format-patch command is generating an email, and
rfc2822 says:
Each line of characters MUST be no more than 998 characters, and
SHOULD be no more than 78 characters, excluding the CRLF.
But we can make the subject arbitrarily long by using header continuation; the line after the "Subject:" should start with whitespace, which indicates to a parser that it is a continuation of the previous header.
You don't show that here:
> From 0c9f8555c55c73fd4e5392c8f8516c389f362d17 Mon Sep 17 00:00:00 2001
> From: Boyang Chen
> Date: Mon, 26 Feb 2024 11:16:00 +0800
> Subject: [PATCH 3/3] Add a file to test, make make sure that the
> message is a bit long but in a single line
but I'm not sure if it's really missing, or if the whitespace got munged as you sent it. Assuming it is, then everything is working as designed.
That said, I have sometimes been annoyed at this myself, because I want to process the mails with tools that are quite capable of handling long lines (e.g., mutt). And doing hacky processing with perl, etc, becomes harder because you have to actually parse the mail correctly rather than just grepping for "^Subject:". ;)
So I have wondered if it would be useful to have a --no-wrap-email option. Or perhaps the existing --no-encode-email-headers should be used as a hint that the user prefers easy-to-parse output over strict rfc compliance.
-Peff
^ permalink raw reply
* Git For Windows, not installing right
From: Chaython Meredith @ 2024-02-26 8:59 UTC (permalink / raw)
To: git@vger.kernel.org
Git for windows, is not setting environmental variables, despite being asked to during installation.
After setting up environmental variables, errors still occur.
git 2.4.4 with all extras selected [unix tools etc] errors out in visual studio code. When trying to clone a repository.
> git clone https://github.com/Chaython/simplewall.git c:\Users\Chay\Git\simplewall --progress
git: 'remote-https' is not a git command. See 'git --help'.
I've tried changing installation settings... Installing from winget... Installing from chocolatey and obviously manually installing the x64 executable. Each time trying BCU/Geek to properly uninstall all leftovers.
I normally clone using github client, however I thought it would both be more convenient to use visual studio and github client didn't automatically import .gitmodules so I could compile the application....
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Christian Couder @ 2024-02-26 9:28 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Phillip Wood, Josh Steadmon, Achu Luma
In-Reply-To: <20240225112722.89221-4-l.s.r@web.de>
On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:
>
> Simplify TEST_CHAR_CLASS by using TEST for each character separately.
> This increases the number of tests to 3598,
Does this mean that when all the tests pass there will be 3598 lines
of output on the terminal instead of 14 before this patch?
If that's the case, I don't like this.
> but avoids the need for
> using internal functions and test_msg() for custom messages. The
> resulting macro has minimal test setup overhead.
Yeah, the code looks definitely cleaner, but a clean output is important too.
Thanks!
^ permalink raw reply
* Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`
From: Johannes Schindelin @ 2024-02-26 9:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <xmqqle7asnu9.fsf@gitster.g>
Hi Junio,
On Fri, 23 Feb 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Some functions in Git's source code follow the convention that returning
> > a negative value indicates a fatal error, e.g. repository corruption.
> >
> > Let's use this convention in `repo_in_merge_bases()` to report when one
> > of the specified commits is missing (i.e. when `repo_parse_commit()`
> > reports an error).
> >
> > Also adjust the callers of `repo_in_merge_bases()` to handle such
> > negative return values.
>
> All of the above makes sense, but I have to wonder if this hunk
> should rather want to be part of the previous step:
>
> > @@ -486,10 +488,10 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
> > timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
> >
> > if (repo_parse_commit(r, commit))
> > - return ret;
> > + return ignore_missing_commits ? 0 : -1;
> > for (i = 0; i < nr_reference; i++) {
> > if (repo_parse_commit(r, reference[i]))
> > - return ret;
> > + return ignore_missing_commits ? 0 : -1;
> >
> > generation = commit_graph_generation(reference[i]);
> > if (generation > max_generation)
>
> as this hunk is not about many callers of repo_in_merge_bases() that
> ignored the return values, which are all fixed by this patch, but
> about returning that error signal back to the caller.
>
> Yes, I know you wrote in [02/11] that it does not change the
> behaviour, and if you move this hunk to [02/11], it might change the
> behaviour, but that is changing for the better.
I wanted 2/11 to be trivial to review, and therefore specifically wanted
behavior not to change just yet: At least when I review patches, this
information helps me assess the correctness of the patch because I have a
different pair of glasses on, so to say.
> Besides, adding a parameter "ignore_missing" to the function only to be
> ignored until the next patch feels rather incomplete.
By that reasoning, the entire patch series should be squashed into a
single patch, as the missing commits will only be handled properly if all
11 patches are applied ;-)
Seriously again, I designed this patch series in a way where it builds up
incrementally, adding preparations here and there, in as easily reviewable
a shape as I could, until the final patch wraps everything in a bow.
I did this mostly to be able to convince myself of the correctness of the
patches because I sense such a vast opportunity for bugs to creep in.
> The other changes in this patch about its primary theme, fixing the
> callers that used to ignore return values of repo_in_merge_bases(),
> all looked sensible. This hunk somehow stood out like a sore thumb
> to me.
I have an idea. How about pulling out this hunk into its own patch? And
insert it between 2/11 and 3/11? That would probably make most sense, as
it would make the patch series still (relatively) easy to review, and it
would not conflate the purpose of this hunk with the rest of 3/11's hunks.
What do you think?
Ciao,
Johannes
^ permalink raw reply
* Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
From: Christian Couder @ 2024-02-26 9:59 UTC (permalink / raw)
To: Eugenio Gigante; +Cc: git, sunshine, gitster
In-Reply-To: <20240224112638.72257-2-giganteeugenio2@gmail.com>
On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
<giganteeugenio2@gmail.com> wrote:
>
> The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
> while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.
It's not clear from the patch that refresh() passes 'flags' as an
argument to refresh_index(), so it might help reviewers a bit if you
could tell that.
> Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
> this commit changes the type of 'flags' to unsigned.
We prefer to use "let's change this and that" or just "change this and
that" rather than "this commit changes this and that", see
https://git-scm.com/docs/SubmittingPatches/#imperative-mood.
It might help if you could add a bit more explanation about why it's a
good thing to use an unsigned variable instead of a signed one. For
example you could say that it documents that we are not doing anything
funny with the MSB.
> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
> builtin/add.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
The patch looks correct, thanks!
^ permalink raw reply
* [PATCH 0/3] show-branch --reflog fixes
From: Jeff King @ 2024-02-26 10:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240222172252.GA3535450@coredump.intra.peff.net>
On Thu, Feb 22, 2024 at 12:22:52PM -0500, Jeff King wrote:
> If none of this makes sense, it is because I am only now untangling what
> is going on with 6436a20284. ;) I will try to polish my proposed patches
> and hopefully that will explain it a bit more clearly (I may not get to
> it until tomorrow though).
OK, so here's what I came up with. One thing I did not realize, as I
was writing my patches directly atop 6436a20284, is that we actually
fixed the reflog message bug back in f2463490c4 (show-branch: show
reflog message, 2021-12-02). And several tests have been added since
then.
So I gave up on trying to build on top of the source of the bug, and
just rebased onto the tip of master. It should apply to recent "maint"
as well, I'd think.
[1/3]: Revert "refs: allow @{n} to work with n-sized reflog"
[2/3]: get_oid_basic(): special-case ref@{n} for oldest reflog entry
[3/3]: read_ref_at(): special-case ref@{0} for an empty reflog
object-name.c | 9 ++++++
refs.c | 65 +++++++++++++++++++-----------------------
refs.h | 15 +++++++++-
t/t3202-show-branch.sh | 49 +++++++++++++++++++++----------
4 files changed, 87 insertions(+), 51 deletions(-)
^ permalink raw reply
* [PATCH 1/3] Revert "refs: allow @{n} to work with n-sized reflog"
From: Jeff King @ 2024-02-26 10:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net>
This reverts commit 6436a20284f33d42103cac93bd82e65bebb31526.
The idea of that commit is that if read_ref_at() is counting back to the
Nth reflog but the reflog is short by one entry (e.g., because it was
pruned), we can find the oid of the missing entry by looking at the
"before" oid value of the entry that comes after it (whereas before, we
looked at the "after" value of each entry and complained that we
couldn't find the one from before the truncation).
This works fine for resolving the oid of ref@{n}, as it is used by
get_oid_basic(), which does not look at any other aspect of the reflog
we found (e.g., its timestamp or message). But there's another caller of
read_ref_at(): in show-branch we use it to walk over the reflog, and we
do care about the reflog entry. And so that commit broke "show-branch
--reflog"; it shows the reflog message for ref@{0} as ref@{1}, ref@{1}
as ref@{2}, and so on.
For example, in the new test in t3202 we produce:
! [branch@{0}] (0 seconds ago) commit: three
! [branch@{1}] (0 seconds ago) commit: three
! [branch@{2}] (60 seconds ago) commit: two
! [branch@{3}] (2 minutes ago) reset: moving to HEAD^
instead of the correct:
! [branch@{0}] (0 seconds ago) commit: three
! [branch@{1}] (60 seconds ago) commit: two
! [branch@{2}] (2 minutes ago) reset: moving to HEAD^
! [branch@{3}] (2 minutes ago) commit: one
But there's another bug, too: because it is looking at the "old" value
of the reflog after the one we're interested in, it has to special-case
ref@{0} (since there isn't anything after it). That's why it doesn't
show the offset bug in the output above. But this special-case code
fails to handle the situation where the reflog is empty or missing; it
returns success even though the reflog message out-parameter has been
left uninitialized. You can't trigger this through get_oid_basic(), but
"show-branch --reflog" will pretty reliably segfault as it tries to
access the garbage pointer.
Fixing the segfault would be pretty easy. But the off-by-one problem is
inherent in this approach. So let's start by reverting the commit to
give us a clean slate to work with.
This isn't a pure revert; all of the code changes are reverted, but for
the tests:
1. We'll flip the cases in t1508 to expect_failure; making these work
was the goal of 6436a2028, and we'll want to use them for our
replacement approach.
2. There's a test in t3202 for "show-branch --reflog", but it expects
the broken output! It was added by f2463490c4 (show-branch: show
reflog message, 2021-12-02) which was fixing another bug, and I
think the author simply didn't notice that the second line showed
the wrong reflog.
Rather than fixing that test, let's replace it with one that is
more thorough (while still covering the reflog message fix from
that commit). We'll use a longer reflog, which lets us see more
entries (thus making the "off by one" pattern much more clear). And
we'll use a more recent timestamp for "now" so that our relative
dates have more resolution. That lets us see that the reflog dates
are correct (whereas when you are 4 years away, two entries that
are 60 seconds apart will have the same "4 years ago" relative
date). Because we're adjusting the repository state, I've moved
this new test to the end of the script, leaving the other tests
undisturbed.
We'll also add a new test which covers the missing reflog case;
previously it segfaulted, but now it reports the empty reflog).
Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
refs.c | 48 +++++++++++--------------------------
t/t1508-at-combinations.sh | 4 ++--
t/t3202-show-branch.sh | 49 ++++++++++++++++++++++++++------------
3 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/refs.c b/refs.c
index c633abf284..ba1a4db754 100644
--- a/refs.c
+++ b/refs.c
@@ -1038,55 +1038,40 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
const char *message, void *cb_data)
{
struct read_ref_at_cb *cb = cb_data;
- int reached_count;
cb->tz = tz;
cb->date = timestamp;
- /*
- * It is not possible for cb->cnt == 0 on the first iteration because
- * that special case is handled in read_ref_at().
- */
- if (cb->cnt > 0)
- cb->cnt--;
- reached_count = cb->cnt == 0 && !is_null_oid(ooid);
- if (timestamp <= cb->at_time || reached_count) {
+ if (timestamp <= cb->at_time || cb->cnt == 0) {
set_read_ref_cutoffs(cb, timestamp, tz, message);
/*
* we have not yet updated cb->[n|o]oid so they still
* hold the values for the previous record.
*/
- if (!is_null_oid(&cb->ooid) && !oideq(&cb->ooid, noid))
- warning(_("log for ref %s has gap after %s"),
+ if (!is_null_oid(&cb->ooid)) {
+ oidcpy(cb->oid, noid);
+ if (!oideq(&cb->ooid, noid))
+ warning(_("log for ref %s has gap after %s"),
cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
- if (reached_count)
- oidcpy(cb->oid, ooid);
- else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)
+ }
+ else if (cb->date == cb->at_time)
oidcpy(cb->oid, noid);
else if (!oideq(noid, cb->oid))
warning(_("log for ref %s unexpectedly ended on %s"),
cb->refname, show_date(cb->date, cb->tz,
DATE_MODE(RFC2822)));
+ cb->reccnt++;
+ oidcpy(&cb->ooid, ooid);
+ oidcpy(&cb->noid, noid);
cb->found_it = 1;
+ return 1;
}
cb->reccnt++;
oidcpy(&cb->ooid, ooid);
oidcpy(&cb->noid, noid);
- return cb->found_it;
-}
-
-static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
- struct object_id *noid,
- const char *email UNUSED,
- timestamp_t timestamp, int tz,
- const char *message, void *cb_data)
-{
- struct read_ref_at_cb *cb = cb_data;
-
- set_read_ref_cutoffs(cb, timestamp, tz, message);
- oidcpy(cb->oid, noid);
- /* We just want the first entry */
- return 1;
+ if (cb->cnt > 0)
+ cb->cnt--;
+ return 0;
}
static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
@@ -1121,11 +1106,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,
cb.cutoff_cnt = cutoff_cnt;
cb.oid = oid;
- if (cb.cnt == 0) {
- refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
- return 0;
- }
-
refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
if (!cb.reccnt) {
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index e841309d0e..3e5f32f604 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -103,14 +103,14 @@ test_expect_success 'create path with @' '
check "@:normal" blob content
check "@:fun@ny" blob content
-test_expect_success '@{1} works with only one reflog entry' '
+test_expect_failure '@{1} works with only one reflog entry' '
git checkout -B newbranch main &&
git reflog expire --expire=now refs/heads/newbranch &&
git commit --allow-empty -m "first after expiration" &&
test_cmp_rev newbranch~ newbranch@{1}
'
-test_expect_success '@{0} works with empty reflog' '
+test_expect_failure '@{0} works with empty reflog' '
git checkout -B newbranch main &&
git reflog expire --expire=now refs/heads/newbranch &&
test_cmp_rev newbranch newbranch@{0}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 6a98b2df76..35f35f8091 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -4,9 +4,6 @@ test_description='test show-branch'
. ./test-lib.sh
-# arbitrary reference time: 2009-08-30 19:20:00
-GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
-
test_expect_success 'error descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
@@ -187,18 +184,6 @@ test_expect_success 'show branch --merge-base with N arguments' '
test_cmp expect actual
'
-test_expect_success 'show branch --reflog=2' '
- sed "s/^> //" >expect <<-\EOF &&
- > ! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10
- > ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10
- > --
- > + [refs/heads/branch10@{0}] branch10
- > ++ [refs/heads/branch10@{1}] initial
- EOF
- git show-branch --reflog=2 >actual &&
- test_cmp actual expect
-'
-
# incompatible options
while read combo
do
@@ -264,4 +249,38 @@ test_expect_success 'error descriptions on orphan branch' '
test_branch_op_in_wt -c new-branch
'
+test_expect_success 'setup reflogs' '
+ test_commit base &&
+ git checkout -b branch &&
+ test_commit one &&
+ git reset --hard HEAD^ &&
+ test_commit two &&
+ test_commit three
+'
+
+test_expect_success '--reflog shows reflog entries' '
+ cat >expect <<-\EOF &&
+ ! [branch@{0}] (0 seconds ago) commit: three
+ ! [branch@{1}] (60 seconds ago) commit: two
+ ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^
+ ! [branch@{3}] (2 minutes ago) commit: one
+ ----
+ + [branch@{0}] three
+ ++ [branch@{1}] two
+ + [branch@{3}] one
+ ++++ [branch@{2}] base
+ EOF
+ # the output always contains relative timestamps; use
+ # a known time to get deterministic results
+ GIT_TEST_DATE_NOW=$test_tick \
+ git show-branch --reflog branch >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--reflog handles missing reflog' '
+ git reflog expire --expire=now branch &&
+ test_must_fail git show-branch --reflog branch 2>err &&
+ grep "log .* is empty" err
+'
+
test_done
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 2/3] get_oid_basic(): special-case ref@{n} for oldest reflog entry
From: Jeff King @ 2024-02-26 10:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net>
The goal of 6436a20284 (refs: allow @{n} to work with n-sized reflog,
2021-01-07) was that if we have "n" entries in a reflog, we should still
be able to resolve ref@{n} by looking at the "old" value of the oldest
entry.
Commit 6436a20284 tried to put the logic into read_ref_at() by shifting
its idea of "n" by one. But we reverted that in the previous commit,
since it led to bugs in other callers which cared about the details of
the reflog entry we found. Instead, let's put the special case into the
caller that resolves @{n}, as it cares only about the oid.
read_ref_at() is even kind enough to return the "old" value from the
final reflog; it just returns "1" to signal to us that we ran off the
end of the reflog. But we can notice in the caller that we read just
enough records for that "old" value to be the one we're looking for, and
use it.
Note that read_ref_at() could notice this case, too, and just return 0.
But we don't want to do that, because the caller must be made aware that
we only found the oid, not an actual reflog entry (and the call sites in
show-branch do care about this).
There is one complication, though. When read_ref_at() hits a truncated
reflog, it will return the "old" value of the oldest entry only if it is
not the null oid. Otherwise, it actually returns the "new" value from
that entry! This bit of fudging is due to d1a4489a56 (avoid null SHA1 in
oldest reflog, 2008-07-08), where asking for "ref@{20.years.ago}" for a
ref created recently will produce the initial value as a convenience
(even though technically it did not exist 20 years ago).
But this convenience is only useful for time-based cutoffs. For
count-based cutoffs, get_oid_basic() has always simply complained about
going too far back:
$ git rev-parse HEAD@{20}
fatal: log for 'HEAD' only has 16 entries
and we should continue to do so, rather than returning a nonsense value
(there's even a test in t1508 already which covers this). So let's have
the d1a4489a56 code kick in only when doing timestamp-based cutoffs.
Signed-off-by: Jeff King <peff@peff.net>
---
object-name.c | 9 +++++++++
refs.c | 2 +-
t/t1508-at-combinations.sh | 2 +-
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/object-name.c b/object-name.c
index 3a2ef5d680..511f09bc0f 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1034,6 +1034,15 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
len, str,
show_date(co_time, co_tz, DATE_MODE(RFC2822)));
}
+ } else if (nth == co_cnt && !is_null_oid(oid)) {
+ /*
+ * We were asked for the Nth reflog (counting
+ * from 0), but there were only N entries.
+ * read_ref_at() will have returned "1" to tell
+ * us it did not find an entry, but it did
+ * still fill in the oid with the "old" value,
+ * which we can use.
+ */
} else {
if (flags & GET_OID_QUIETLY) {
exit(128);
diff --git a/refs.c b/refs.c
index ba1a4db754..6b826b002e 100644
--- a/refs.c
+++ b/refs.c
@@ -1083,7 +1083,7 @@ static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid
set_read_ref_cutoffs(cb, timestamp, tz, message);
oidcpy(cb->oid, ooid);
- if (is_null_oid(cb->oid))
+ if (cb->at_time && is_null_oid(cb->oid))
oidcpy(cb->oid, noid);
/* We just want the first entry */
return 1;
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 3e5f32f604..370bf7137e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -103,7 +103,7 @@ test_expect_success 'create path with @' '
check "@:normal" blob content
check "@:fun@ny" blob content
-test_expect_failure '@{1} works with only one reflog entry' '
+test_expect_success '@{1} works with only one reflog entry' '
git checkout -B newbranch main &&
git reflog expire --expire=now refs/heads/newbranch &&
git commit --allow-empty -m "first after expiration" &&
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Jeff King @ 2024-02-26 10:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net>
The previous commit special-cased get_oid_basic()'s handling of ref@{n}
for a reflog with n entries. But its special case doesn't work for
ref@{0} in an empty reflog, because read_ref_at() dies when it notices
the empty reflog!
We can make this work by special-casing this in read_ref_at(). It's
somewhat gross, for two reasons:
1. We have no reflog entry to describe in the "msg" out-parameter. So
we have to leave it uninitialized or make something up.
2. Likewise, we have no oid to put in the "oid" out-parameter. Leaving
it untouched is actually the best thing here, as all of the callers
will have initialized it with the current ref value via
repo_dwim_log(). This is rather subtle, but it is how things worked
in 6436a20284 (refs: allow @{n} to work with n-sized reflog,
2021-01-07) before we reverted it.
The key difference from 6436a20284 here is that we'll return "1" to
indicate that we _didn't_ find the requested reflog entry. Coupled with
the special-casing in get_oid_basic() in the previous commit, that's
enough to make looking up ref@{0} work, and we can flip 6436a20284's
test back to expect_success.
It also means that the call in show-branch which segfaulted with
6436a20284 (and which is now tested in t3202) remains OK. The caller
notices that we could not find any reflog entry, and so it breaks out of
its loop, showing nothing. This is different from the current behavior
of producing an error, but it's just as reasonable (and is exactly what
we'd do if you asked it to walk starting at ref@{1} but there was only 1
entry).
Thus nobody should actually look at the reflog entry info we return. But
we'll still put in some fake values just to be on the safe side, since
this is such a subtle and confusing interface. Likewise, we'll document
what's going on in a comment above the function declaration. If this
were a function with a lot of callers, the footgun would probably not be
worth it. But it has only ever had two callers in its 18-year existence,
and it seems unlikely to grow more. So let's hold our noses and let
users enjoy the convenience of a simulated ref@{0}.
Signed-off-by: Jeff King <peff@peff.net>
---
refs.c | 15 +++++++++++++++
refs.h | 15 ++++++++++++++-
t/t1508-at-combinations.sh | 2 +-
t/t3202-show-branch.sh | 4 ++--
4 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 6b826b002e..8eaec5eca7 100644
--- a/refs.c
+++ b/refs.c
@@ -1109,6 +1109,21 @@ int read_ref_at(struct ref_store *refs, const char *refname,
refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
if (!cb.reccnt) {
+ if (cnt == 0) {
+ /*
+ * The caller asked for ref@{0}, and we had no entries.
+ * It's a bit subtle, but in practice all callers have
+ * prepped the "oid" field with the current value of
+ * the ref, which is the most reasonable fallback.
+ *
+ * We'll put dummy values into the out-parameters (so
+ * they're not just uninitialized garbage), and the
+ * caller can take our return value as a hint that
+ * we did not find any such reflog.
+ */
+ set_read_ref_cutoffs(&cb, 0, 0, "empty reflog");
+ return 1;
+ }
if (flags & GET_OID_QUIETLY)
exit(128);
else
diff --git a/refs.h b/refs.h
index 303c5fac4d..37116ad2b2 100644
--- a/refs.h
+++ b/refs.h
@@ -440,7 +440,20 @@ int refs_create_reflog(struct ref_store *refs, const char *refname,
struct strbuf *err);
int safe_create_reflog(const char *refname, struct strbuf *err);
-/** Reads log for the value of ref during at_time. **/
+/**
+ * Reads log for the value of ref during at_time (in which case "cnt" should be
+ * negative) or the reflog "cnt" entries from the top (in which case "at_time"
+ * should be 0).
+ *
+ * If we found the reflog entry in question, returns 0 (and details of the
+ * entry can be found in the out-parameters).
+ *
+ * If we ran out of reflog entries, the out-parameters are filled with the
+ * details of the oldest entry we did find, and the function returns 1. Note
+ * that there is one important special case here! If the reflog was empty
+ * and the caller asked for the 0-th cnt, we will return "1" but leave the
+ * "oid" field untouched.
+ **/
int read_ref_at(struct ref_store *refs,
const char *refname, unsigned int flags,
timestamp_t at_time, int cnt,
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 370bf7137e..e841309d0e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -110,7 +110,7 @@ test_expect_success '@{1} works with only one reflog entry' '
test_cmp_rev newbranch~ newbranch@{1}
'
-test_expect_failure '@{0} works with empty reflog' '
+test_expect_success '@{0} works with empty reflog' '
git checkout -B newbranch main &&
git reflog expire --expire=now refs/heads/newbranch &&
test_cmp_rev newbranch newbranch@{0}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 35f35f8091..a1139f79e2 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -279,8 +279,8 @@ test_expect_success '--reflog shows reflog entries' '
test_expect_success '--reflog handles missing reflog' '
git reflog expire --expire=now branch &&
- test_must_fail git show-branch --reflog branch 2>err &&
- grep "log .* is empty" err
+ git show-branch --reflog branch >actual &&
+ test_must_be_empty actual
'
test_done
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Jeff King @ 2024-02-26 10:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240226100803.GC2685600@coredump.intra.peff.net>
On Mon, Feb 26, 2024 at 05:08:03AM -0500, Jeff King wrote:
> Thus nobody should actually look at the reflog entry info we return. But
> we'll still put in some fake values just to be on the safe side, since
> this is such a subtle and confusing interface. Likewise, we'll document
> what's going on in a comment above the function declaration. If this
> were a function with a lot of callers, the footgun would probably not be
> worth it. But it has only ever had two callers in its 18-year existence,
> and it seems unlikely to grow more. So let's hold our noses and let
> users enjoy the convenience of a simulated ref@{0}.
Obviously I'm sympathetic to Patrick's position that this empty-reflog
special case is kind of gross. ;)
That's one of the reasons I split this out from patch 2; we can see
exactly what must be done to make each case work. And in fact I had
originally started to write a patch that simply changed t1508 to expect
failure. I could still be persuaded to go that way if anybody feels
strongly.
-Peff
^ permalink raw reply
* Re: [GSOC][RFC PATCH 1/1] t: t7301-clean-interactive: Use test_path_is_(missing|file)
From: Christian Couder @ 2024-02-26 10:13 UTC (permalink / raw)
To: Vincenzo Mezzela; +Cc: git
In-Reply-To: <20240219172214.7644-2-vincenzo.mezzela@gmail.com>
About the subject, instead of "t: t7301-clean-interactive: Use ...",
something like "t7301-clean-interactive: use ..." or "t7301: use ..."
would be better. First because there is no need for both "t:" and
"t7301-clean-interactive:" for the "area" part of the subject. Only
one "area" part is enough. Second because the first word after the
"area" part should not be capitalized. See:
https://git-scm.com/docs/SubmittingPatches/#summary-section
On Mon, Feb 19, 2024 at 6:22 PM Vincenzo Mezzela
<vincenzo.mezzela@gmail.com> wrote:
>
> Replace test -(f|e) with the appropriate helper functions provided by
> test-lib-functions.sh
I think your commit message should explain why it's better to use
test_path_is_(missing|file) instead of test -(f|e).
Also replacing `test ! -f` with `test_path_is_missing` might be wrong
if it's Ok to have a directory instead of a file (in which case the
latter would fail while the former would work). So a few words about
why it's Ok to do it here would be nice.
Thanks!
^ permalink raw reply
* [PATCH] userdiff: skip textconv caching when not in a repository
From: Jeff King @ 2024-02-26 10:27 UTC (permalink / raw)
To: Paweł Dominiak; +Cc: git
In-Reply-To: <CACeVQwRkoJejrWvym_BAZE-OWi+q3RZfyEpwAwA+0hGUBCeD=Q@mail.gmail.com>
On Mon, Feb 26, 2024 at 08:56:09AM +0100, Paweł Dominiak wrote:
> But it seems like something git should handle on its own, so that diff
> would accommodate use in different circumstances with the same config.
Yes, definitely. Here's my patch with a commit message and a test.
-- >8 --
Subject: userdiff: skip textconv caching when not in a repository
The textconv caching system uses git-notes to store its cache entries.
But if you're using "diff --no-index" outside of a repository, then
obviously that isn't going to work.
Since caching is just an optimization, it's OK for us to skip it.
However, the current behavior is much worse: we call notes_cache_init()
which tries to look up the ref, and the low-level ref code hits a BUG(),
killing the program. Instead, we should notice before setting up the
cache that it there's no repository, and just silently skip it.
Reported-by: Paweł Dominiak <dominiak.pawel@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
t/t4042-diff-textconv-caching.sh | 22 ++++++++++++++++++++++
userdiff.c | 4 +++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index bf33aedf4b..8ebfa3c1be 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -118,4 +118,26 @@ test_expect_success 'log notes cache and still use cache for -p' '
git log --no-walk -p refs/notes/textconv/magic HEAD
'
+test_expect_success 'caching is silently ignored outside repo' '
+ mkdir -p non-repo &&
+ echo one >non-repo/one &&
+ echo two >non-repo/two &&
+ echo "* diff=test" >attr &&
+ test_expect_code 1 \
+ nongit git -c core.attributesFile="$PWD/attr" \
+ -c diff.test.textconv="tr a-z A-Z <" \
+ -c diff.test.cachetextconv=true \
+ diff --no-index one two >actual &&
+ cat >expect <<-\EOF &&
+ diff --git a/one b/two
+ index 5626abf..f719efd 100644
+ --- a/one
+ +++ b/two
+ @@ -1 +1 @@
+ -ONE
+ +TWO
+ EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/userdiff.c b/userdiff.c
index e399543823..fce3a31efa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -3,6 +3,7 @@
#include "userdiff.h"
#include "attr.h"
#include "strbuf.h"
+#include "environment.h"
static struct userdiff_driver *drivers;
static int ndrivers;
@@ -460,7 +461,8 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,
if (!driver->textconv)
return NULL;
- if (driver->textconv_want_cache && !driver->textconv_cache) {
+ if (driver->textconv_want_cache && !driver->textconv_cache &&
+ have_git_dir()) {
struct notes_cache *c = xmalloc(sizeof(*c));
struct strbuf name = STRBUF_INIT;
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* Re: Interactive rebase: using "pick" for merge commits
From: Phillip Wood @ 2024-02-26 10:56 UTC (permalink / raw)
To: Stefan Haller, phillip.wood, Patrick Steinhardt; +Cc: git
In-Reply-To: <2739325d-93b1-445c-aac9-3e0ec54a27e4@haller-berlin.de>
Hi Stefan
On 23/02/2024 20:59, Stefan Haller wrote:
> On 12.02.24 15:38, Phillip Wood wrote:
>> Hi Patrick and Stefan
>>
>> It would certainly be possible to extend the sequencer to do that but
>> I'm not familiar with why people use "git cherry-pick -m" [1] so I'm
>> wondering what this would be used for. It would involve a bit of extra
>> complexity so I think we'd want a compelling reason as to why
>> cherry-picking merges without maintaining the topology is useful
>> especially as one can currently do that via "exec git cherry-pick -m ..."
>
> Ok, I suppose the answer will probably not count as a compelling reason.
> My reason for wanting this is that lazygit currently implements
> cherry-picking in terms of an interactive rebase, rather then calling
> git-cherry-pick. And the reason why it does this is that when you
> cherry-pick multiple commits, and one of them conflicts, then you get
> lazygit's nice visualization of the rebase todo list to show you where
> in the sequence you are, what the conflicting commit is, how many are
> left etc. It just happens to support this well for
> .git/rebase-merge/git-rebase-todo, but not for .git/sequencer/todo.
Thanks for the context. I can see how that is convenient for lazygit
(and makes we think that perhaps we should teach "git status" to show
pending cherry-picks) but I'm afraid I don't think that is a good reason
for adding the ability to pick merges to git rebase.
> It probably makes more sense to teach lazygit to visualize the
> .git/sequencer/todo file, and then use git cherry-pick.
If lazygit is generating the todo list for the cherry-pick could it
check if the commit is a merge and insert "exec cherry-pick -m ..." for
those commits? The UI could detect that and display something more user
friendly for those lines in the todo list. It is still more work for
lazygit but perhaps less than supporting cherry-picks directly.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH] rebase -i: improve error message when picking merge
From: Phillip Wood via GitGitGadget @ 2024-02-26 10:58 UTC (permalink / raw)
To: git; +Cc: Stefan Haller, Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
The only todo commands that accept a merge commit are "merge" and
"reset". All the other commands like "pick" or "reword" fail when they
try to pick a a merge commit and print the message
error: commit abc123 is a merge but no -m option was given.
followed by a hint about the command being rescheduled. This message is
designed to help the user when they cherry-pick a merge and forget to
pass "-m". For users who are rebasing the message is confusing as there
is no way for rebase to cherry-pick the merge.
Improve the user experience by detecting the error when the todo list is
parsed rather than waiting for the "pick" command to fail and print a
message recommending the "merge" command instead. We recommend "merge"
rather than "exec git cherry-pick -m ..." on the assumption that
cherry-picking merges is relatively rare and it is more likely that the
user chose "pick" by a mistake.
It would be possible to support cherry-picking merges by allowing the
user to pass "-m" to "pick" commands but that adds complexity to do
something that can already be achieved with
exec git cherry-pick -m1 abc123
The change is relatively straight forward but is complicated slightly as
we now need to tell the parser if we're rebasing or not.
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
rebase -i: improve error message when picking merge
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1672
builtin/rebase.c | 2 +-
rebase-interactive.c | 7 ++---
sequencer.c | 49 ++++++++++++++++++++++++++++++-----
sequencer.h | 2 +-
t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++
5 files changed, 81 insertions(+), 12 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b086f651a6..a33e41c44da 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
else {
discard_index(&the_index);
if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
- &todo_list))
+ &todo_list, 1))
BUG("unusable todo list");
ret = complete_action(the_repository, &replay, flags,
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3d..78d5ed1a41d 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
* it. If there is an error, we do not return, because the user
* might want to fix it in the first place. */
if (!initial)
- incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
+ incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,
+ todo_list, 1) |
file_exists(rebase_path_dropped());
if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
@@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
if (initial && new_todo->buf.len == 0)
return -3;
- if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
+ if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {
fprintf(stderr, _(edit_todo_list_advice));
return -4;
}
@@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_
int res = 0;
if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
- todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
+ todo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);
res = todo_list_check(&backup, todo_list);
}
diff --git a/sequencer.c b/sequencer.c
index 91de546b323..cf808c24d20 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
return 0;
}
+static int error_merge_commit(enum todo_command command)
+{
+ switch(command) {
+ case TODO_PICK:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s'"),
+ todo_command_info[command].str, "merge -C");
+
+ case TODO_REWORD:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s'"),
+ todo_command_info[command].str, "merge -c");
+
+ case TODO_EDIT:
+ return error(_("'%s' does not accept merge commits, "
+ "please use '%s' followed by '%s'"),
+ todo_command_info[command].str,
+ "merge -C", "break");
+
+ case TODO_FIXUP:
+ case TODO_SQUASH:
+ return error(_("cannot squash merge commit into another commit"));
+
+ default:
+ BUG("unexpected todo_command");
+ }
+}
+
static int parse_insn_line(struct repository *r, struct todo_item *item,
- const char *buf, const char *bol, char *eol)
+ const char *buf, const char *bol, char *eol,
+ int rebasing)
{
struct object_id commit_oid;
char *end_of_object_name;
@@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
return status;
item->commit = lookup_commit_reference(r, &commit_oid);
- return item->commit ? 0 : -1;
+ if (!item->commit)
+ return -1;
+ if (rebasing && item->command != TODO_MERGE &&
+ item->commit->parents && item->commit->parents->next)
+ return error_merge_commit(item->command);
+ return 0;
}
int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
@@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
}
int todo_list_parse_insn_buffer(struct repository *r, char *buf,
- struct todo_list *todo_list)
+ struct todo_list *todo_list, int rebasing)
{
struct todo_item *item;
char *p = buf, *next_p;
@@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
item = append_new_todo(todo_list);
item->offset_in_buf = p - todo_list->buf.buf;
- if (parse_insn_line(r, item, buf, p, eol)) {
+ if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
res = error(_("invalid line %d: %.*s"),
i, (int)(eol - p), p);
item->command = TODO_COMMENT + 1;
@@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
return -1;
- res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
+ res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
+ is_rebase_i(opts));
if (res) {
if (is_rebase_i(opts))
return error(_("please fix this using "
@@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
struct todo_list done = TODO_LIST_INIT;
if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
- !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
+ !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
todo_list->done_nr = count_commands(&done);
else
todo_list->done_nr = 0;
@@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
strbuf_release(&buf2);
/* Nothing is done yet, and we're reparsing, so let's reset the count */
new_todo.total_nr = 0;
- if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
+ if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
BUG("invalid todo list after expanding IDs:\n%s",
new_todo.buf.buf);
diff --git a/sequencer.h b/sequencer.h
index dcef7bb99c0..ed2c4b38514 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -136,7 +136,7 @@ struct todo_list {
}
int todo_list_parse_insn_buffer(struct repository *r, char *buf,
- struct todo_list *todo_list);
+ struct todo_list *todo_list, int rebasing);
int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
const char *file, const char *shortrevisions,
const char *shortonto, int num, unsigned flags);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 64b641002e4..20b8589ad07 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
test_path_is_missing execed
'
+test_expect_success 'non-merge commands reject merge commits' '
+ test_when_finished "test_might_fail git rebase --abort" &&
+ git checkout E &&
+ git merge I &&
+ oid=$(git rev-parse HEAD) &&
+ cat >todo <<-EOF &&
+ pick $oid
+ reword $oid
+ edit $oid
+ fixup $oid
+ squash $oid
+ EOF
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i HEAD 2>actual
+ ) &&
+ cat >expect <<-EOF &&
+ error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
+ error: invalid line 1: pick $oid
+ error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
+ error: invalid line 2: reword $oid
+ error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
+ error: invalid line 3: edit $oid
+ error: cannot squash merge commit into another commit
+ error: invalid line 4: fixup $oid
+ error: cannot squash merge commit into another commit
+ error: invalid line 5: squash $oid
+ You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
+ Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
+ EOF
+ test_cmp expect actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox