* [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: brianmlyles @ 2024-01-19 5:59 UTC (permalink / raw)
To: git; +Cc: me, newren, Brian Lyles
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
From: Brian Lyles <brianmlyles@gmail.com>
As with `git-rebase` and `git-am`, `git-cherry-pick` can result in a
commit being made redundant if the content from the picked commit is
already present in the target history. However, `git-cherry-pick` does
not have the same options available that `git-rebase` and `git-am` have.
There are three things that can be done with these redundant commits:
drop them, keep them, or have the cherry-pick stop and wait for the user
to take an action. `git-rebase` has the `--empty` option added in commit
e98c4269c8 (rebase (interactive-backend): fix handling of commits that
become empty, 2020-02-15), which handles all three of these scenarios.
Similarly, `git-am` got its own `--empty` in 7c096b8d61 (am: support
--empty=<option> to handle empty patches, 2021-12-09).
`git-cherry-pick`, on the other hand, only supports two of the three
possiblities: Keep the redundant commits via `--keep-redundant-commits`,
or have the cherry-pick fail by not specifying that option. There is no
way to automatically drop redundant commits.
In order to bring `git-cherry-pick` more in-line with `git-rebase` and
`git-am`, this commit adds an `--empty` option to `git-cherry-pick`. It
has the same three options (keep, drop, and stop), and largely behaves
the same. The notable difference is that for `git-cherry-pick`, the
default will be `stop`, which maintains the current behavior when the
option is not specified.
The `--keep-redundant-commits` option will be documented as a deprecated
synonym of `--empty=keep`, and will be supported for backwards
compatibility for the time being.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
Documentation/git-cherry-pick.txt | 28 ++++++++++++++++++-------
builtin/revert.c | 35 ++++++++++++++++++++++++++++++-
sequencer.c | 6 ++++++
t/t3505-cherry-pick-empty.sh | 26 ++++++++++++++++++++++-
4 files changed, 86 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 806295a730..8c20a10d4b 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -132,23 +132,37 @@ effect to your index in a row.
keeps commits that were initially empty (i.e. the commit recorded the
same tree as its parent). Commits which are made empty due to a
previous commit will cause the cherry-pick to fail. To force the
- inclusion of those commits use `--keep-redundant-commits`.
+ inclusion of those commits use `--empty=keep`.
--allow-empty-message::
By default, cherry-picking a commit with an empty message will fail.
This option overrides that behavior, allowing commits with empty
messages to be cherry picked.
---keep-redundant-commits::
- If a commit being cherry picked duplicates a commit already in the
- current history, it will become empty. By default these
- redundant commits cause `cherry-pick` to stop so the user can
- examine the commit. This option overrides that behavior and
- creates an empty commit object. Note that use of this option only
+--empty=(stop|drop|keep)::
+ How to handle commits being cherry-picked that are redundant with
+ changes already in the current history.
++
+--
+`stop`;;
+ The cherry-pick will stop when the empty commit is applied, allowing
+ you to examine the commit. This is the default behavior.
+`drop`;;
+ The empty commit will be dropped.
+`keep`;;
+ The empty commit will be kept. Note that use of this option only
results in an empty commit when the commit was not initially empty,
but rather became empty due to a previous commit. Commits that were
initially empty will cause the cherry-pick to fail. To force the
inclusion of those commits use `--allow-empty`.
+--
++
+Note that commits which start empty will cause the cherry-pick to fail (unless
+`--allow-empty` is specified).
++
+
+--keep-redundant-commits::
+ Deprecated synonym for `--empty=keep`.
--strategy=<strategy>::
Use the given merge strategy. Should only be used once.
diff --git a/builtin/revert.c b/builtin/revert.c
index b2cfde7a87..1491c45e26 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -45,6 +45,30 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
}
+enum empty_action {
+ STOP_ON_EMPTY_COMMIT = 0, /* output errors and stop in the middle of a cherry-pick */
+ DROP_EMPTY_COMMIT, /* skip with a notice message */
+ KEEP_EMPTY_COMMIT, /* keep recording as empty commits */
+};
+
+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
+{
+ int *opt_value = opt->value;
+
+ BUG_ON_OPT_NEG(unset);
+
+ if (!strcmp(arg, "stop"))
+ *opt_value = STOP_ON_EMPTY_COMMIT;
+ else if (!strcmp(arg, "drop"))
+ *opt_value = DROP_EMPTY_COMMIT;
+ else if (!strcmp(arg, "keep"))
+ *opt_value = KEEP_EMPTY_COMMIT;
+ else
+ return error(_("invalid value for '%s': '%s'"), "--empty", arg);
+
+ return 0;
+}
+
static int option_parse_m(const struct option *opt,
const char *arg, int unset)
{
@@ -87,6 +111,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
+ enum empty_action empty_opt;
int cmd = 0;
struct option base_options[] = {
OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
@@ -116,7 +141,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
- OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+ OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("deprecated: use --empty=keep instead")),
+ OPT_CALLBACK_F(0, "empty", &empty_opt, "(stop|drop|keep)",
+ N_("how to handle commits that become empty"),
+ PARSE_OPT_NONEG, parse_opt_empty),
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
@@ -136,6 +164,11 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
+ if (opts->action == REPLAY_PICK) {
+ opts->drop_redundant_commits = (empty_opt == DROP_EMPTY_COMMIT);
+ opts->keep_redundant_commits = opts->keep_redundant_commits || (empty_opt == KEEP_EMPTY_COMMIT);
+ }
+
if (cleanup_arg) {
opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
opts->explicit_cleanup = 1;
diff --git a/sequencer.c b/sequencer.c
index 582bde8d46..c49c27c795 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2934,6 +2934,9 @@ static int populate_opts_cb(const char *key, const char *value,
else if (!strcmp(key, "options.allow-empty-message"))
opts->allow_empty_message =
git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
+ else if (!strcmp(key, "options.drop-redundant-commits"))
+ opts->drop_redundant_commits =
+ git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
else if (!strcmp(key, "options.keep-redundant-commits"))
opts->keep_redundant_commits =
git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
@@ -3478,6 +3481,9 @@ static int save_opts(struct replay_opts *opts)
if (opts->allow_empty_message)
res |= git_config_set_in_file_gently(opts_file,
"options.allow-empty-message", "true");
+ if (opts->drop_redundant_commits)
+ res |= git_config_set_in_file_gently(opts_file,
+ "options.drop-redundant-commits", "true");
if (opts->keep_redundant_commits)
res |= git_config_set_in_file_gently(opts_file,
"options.keep-redundant-commits", "true");
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 6adfd25351..ae0cf7886a 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -89,7 +89,7 @@ test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
git commit -m "add file2 on the side"
'
-test_expect_success 'cherry-pick a no-op without --keep-redundant' '
+test_expect_success 'cherry-pick a no-op with neither --keep-redundant nor --empty' '
git reset --hard &&
git checkout fork^0 &&
test_must_fail git cherry-pick main
@@ -104,4 +104,28 @@ test_expect_success 'cherry-pick a no-op with --keep-redundant' '
test_cmp expect actual
'
+test_expect_success 'cherry-pick a no-op with --empty=ask' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ test_must_fail git cherry-pick --empty=ask main
+'
+
+test_expect_success 'cherry-pick a no-op with --empty=drop' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ git cherry-pick --empty=drop main &&
+ git show -s --format=%s >actual &&
+ echo "add file2 on the side" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick a no-op with --empty=keep' '
+ git reset --hard &&
+ git checkout fork^0 &&
+ git cherry-pick --empty=keep main &&
+ git show -s --format=%s >actual &&
+ echo "add file2 on main" >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.41.0
^ permalink raw reply related
* [PATCH 3/4] rebase: Update `--empty=ask` to `--empty=drop`
From: brianmlyles @ 2024-01-19 5:59 UTC (permalink / raw)
To: git; +Cc: me, newren, Brian Lyles
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
From: Brian Lyles <brianmlyles@gmail.com>
When `git-am` got its own `--empty` option in 7c096b8d61 (am: support
--empty=<option> to handle empty patches, 2021-12-09), `stop` was used
instead of `ask`. `stop` is a more accurate term for describing what
really happens, and consistency is good. This commit updates
`git-rebase` to also use `stop`, while keeping `ask` as a deprecated
synonym.
In a future commit, we'll be adding a new `--empty` option for
`git-cherry-pick` as well, making the consistency even more relevant.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
Documentation/git-rebase.txt | 8 +++++---
builtin/rebase.c | 12 ++++++------
t/t3424-rebase-empty.sh | 17 ++++++++++++++---
3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3ee85f6d86..fe74d0c367 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -289,7 +289,7 @@ See also INCOMPATIBLE OPTIONS below.
+
See also INCOMPATIBLE OPTIONS below.
---empty=(drop|keep|ask)::
+--empty=(drop|keep|stop)::
How to handle commits that are not empty to start and are not
clean cherry-picks of any upstream commit, but which become
empty after rebasing (because they contain a subset of already
@@ -300,12 +300,14 @@ See also INCOMPATIBLE OPTIONS below.
The empty commit will be dropped. This is the default behavior.
`keep`;;
The empty commit will be kept.
-`ask`;;
+`stop`;;
The rebase will halt when the empty commit is applied, allowing you to
choose whether to drop it, edit files more, or just commit the empty
changes. This option is implied when `--interactive` is specified.
Other options, like `--exec`, will use the default of drop unless
`-i`/`--interactive` is explicitly specified.
+`ask`;;
+ A deprecated synonym of `stop`.
--
+
Note that commits which start empty are kept (unless `--no-keep-empty`
@@ -702,7 +704,7 @@ be dropped automatically with `--no-keep-empty`).
Similar to the apply backend, by default the merge backend drops
commits that become empty unless `-i`/`--interactive` is specified (in
which case it stops and asks the user what to do). The merge backend
-also has an `--empty=(drop|keep|ask)` option for changing the behavior
+also has an `--empty=(drop|keep|stop)` option for changing the behavior
of handling commits that become empty.
Directory rename detection
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 043c65dccd..1fb9d8263d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -62,7 +62,7 @@ enum empty_type {
EMPTY_UNSPECIFIED = -1,
EMPTY_DROP,
EMPTY_KEEP,
- EMPTY_ASK
+ EMPTY_STOP
};
enum action {
@@ -963,10 +963,10 @@ static enum empty_type parse_empty_value(const char *value)
return EMPTY_DROP;
else if (!strcasecmp(value, "keep"))
return EMPTY_KEEP;
- else if (!strcasecmp(value, "ask"))
- return EMPTY_ASK;
+ else if (!strcasecmp(value, "stop") || !strcasecmp(value, "ask"))
+ return EMPTY_STOP;
- die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
+ die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"stop\"."), value);
}
static int parse_opt_keep_empty(const struct option *opt, const char *arg,
@@ -1145,7 +1145,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
"instead of ignoring them"),
1, PARSE_OPT_HIDDEN),
OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
- OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|ask)",
+ OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|stop)",
N_("how to handle commits that become empty"),
PARSE_OPT_NONEG, parse_opt_empty),
OPT_CALLBACK_F('k', "keep-empty", &options, NULL,
@@ -1562,7 +1562,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.empty == EMPTY_UNSPECIFIED) {
if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
- options.empty = EMPTY_ASK;
+ options.empty = EMPTY_STOP;
else if (options.exec.nr > 0)
options.empty = EMPTY_KEEP;
else
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index 5e1045a0af..e3ddec88a2 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -72,6 +72,17 @@ test_expect_success 'rebase --merge --empty=keep' '
test_cmp expect actual
'
+test_expect_success 'rebase --merge --empty=stop' '
+ git checkout -B testing localmods &&
+ test_must_fail git rebase --merge --empty=stop upstream &&
+
+ git rebase --skip &&
+
+ test_write_lines D C B A >expect &&
+ git log --format=%s >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'rebase --merge --empty=ask' '
git checkout -B testing localmods &&
test_must_fail git rebase --merge --empty=ask upstream &&
@@ -101,9 +112,9 @@ test_expect_success 'rebase --interactive --empty=keep' '
test_cmp expect actual
'
-test_expect_success 'rebase --interactive --empty=ask' '
+test_expect_success 'rebase --interactive --empty=stop' '
git checkout -B testing localmods &&
- test_must_fail git rebase --interactive --empty=ask upstream &&
+ test_must_fail git rebase --interactive --empty=stop upstream &&
git rebase --skip &&
@@ -112,7 +123,7 @@ test_expect_success 'rebase --interactive --empty=ask' '
test_cmp expect actual
'
-test_expect_success 'rebase --interactive uses default of --empty=ask' '
+test_expect_success 'rebase --interactive uses default of --empty=stop' '
git checkout -B testing localmods &&
test_must_fail git rebase --interactive upstream &&
--
2.41.0
^ permalink raw reply related
* [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`
From: brianmlyles @ 2024-01-19 5:59 UTC (permalink / raw)
To: git; +Cc: me, newren, Brian Lyles
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
From: Brian Lyles <brianmlyles@gmail.com>
Both of these pages document very similar `--empty` options, but with
different styles. This commit aims to make them more consistent.
In a future commit, we'll be documenting a new `--empty` option for
`git-cherry-pick`, making the consistency even more relevant.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
Documentation/git-am.txt | 18 ++++++++++++------
Documentation/git-rebase.txt | 17 ++++++++++++-----
2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index e080458d6c..77df5e606a 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -67,12 +67,18 @@ OPTIONS
This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).
--empty=(stop|drop|keep)::
- By default, or when the option is set to 'stop', the command
- errors out on an input e-mail message lacking a patch
- and stops in the middle of the current am session. When this
- option is set to 'drop', skip such an e-mail message instead.
- When this option is set to 'keep', create an empty commit,
- recording the contents of the e-mail message as its log.
+ How to handle an e-mail message lacking a patch:
++
+--
+`stop`;;
+ The command will fail, stopping in the middle of the current `am`
+ session. This is the default behavior.
+`drop`;;
+ The e-mail message will be skipped.
+`keep`;;
+ An empty commit will be created, with the contents of the e-mail
+ message as its log.
+--
-m::
--message-id::
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index b4526ca246..3ee85f6d86 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -293,13 +293,20 @@ See also INCOMPATIBLE OPTIONS below.
How to handle commits that are not empty to start and are not
clean cherry-picks of any upstream commit, but which become
empty after rebasing (because they contain a subset of already
- upstream changes). With drop (the default), commits that
- become empty are dropped. With keep, such commits are kept.
- With ask (implied by `--interactive`), the rebase will halt when
- an empty commit is applied allowing you to choose whether to
- drop it, edit files more, or just commit the empty changes.
+ upstream changes):
++
+--
+`drop`;;
+ The empty commit will be dropped. This is the default behavior.
+`keep`;;
+ The empty commit will be kept.
+`ask`;;
+ The rebase will halt when the empty commit is applied, allowing you to
+ choose whether to drop it, edit files more, or just commit the empty
+ changes. This option is implied when `--interactive` is specified.
Other options, like `--exec`, will use the default of drop unless
`-i`/`--interactive` is explicitly specified.
+--
+
Note that commits which start empty are kept (unless `--no-keep-empty`
is specified), and commits which are clean cherry-picks (as determined
--
2.41.0
^ permalink raw reply related
* [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: brianmlyles @ 2024-01-19 5:59 UTC (permalink / raw)
To: git; +Cc: me, newren, Brian Lyles
From: Brian Lyles <brianmlyles@gmail.com>
Previously, a consumer of the sequencer that wishes to take advantage of
either the `keep_redundant_commits` or `drop_redundant_commits` feature
must also specify `allow_empty`.
The only consumer of `drop_redundant_commits` is `git-rebase`, which
already allows empty commits by default and simply always enables
`allow_empty`. `keep_redundant_commits` was also consumed by
`git-cherry-pick`, which had to specify `allow-empty` when
`keep_redundant_commits` was specified in order for the sequencer's
`allow_empty()` to actually respect `keep_redundant_commits`.
The latter is an interesting case: As noted in the docs, this means that
`--keep-redundant-commits` implies `--allow-empty`, despite the two
having distinct, non-overlapping meanings:
- `allow_empty` refers specifically to commits which start empty, as
indicated by the documentation for `--allow-empty` within
`git-cherry-pick`:
"Note also, that use of this option only keeps commits that were
initially empty (i.e. the commit recorded the same tree as its
parent). Commits which are made empty due to a previous commit are
dropped. To force the inclusion of those commits use
--keep-redundant-commits."
- `keep_redundant_commits` refers specifically to commits that do not
start empty, but become empty due to the content already existing in
the target history. This is indicated by the documentation for
`--keep-redundant-commits` within `git-cherry-pick`:
"If a commit being cherry picked duplicates a commit already in the
current history, it will become empty. By default these redundant
commits cause cherry-pick to stop so the user can examine the commit.
This option overrides that behavior and creates an empty commit
object. Implies --allow-empty."
This implication of `--allow-empty` therefore seems incorrect: One
should be able to keep a commit that becomes empty without also being
forced to pick commits that start as empty. However, today, the
following series of commands would result in both the commit that became
empty and the commit that started empty being picked despite only
`--keep-redundant-commits` being specified:
git init
echo "a" >test
git add test
git commit -m "Initial commit"
echo "b" >test
git commit -am "a -> b"
git commit --allow-empty -m "empty"
git cherry-pick --keep-redundant-commits HEAD^ HEAD
The same cherry-pick with `--allow-empty` would fail on the redundant
commit, and with neither option would fail on the empty commit.
In a future commit, an `--empty` option will be added to
`git-cherry-pick`, meaning that `drop_redundant_commits` will be
available in that command. For that to be possible with the current
implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
would need to specify `allow_empty` with `drop_redundant_commits` as
well, which is an even less intuitive implication of `--allow-empty`: in
order to prevent redundant commits automatically, initially-empty
commits would need to be kept automatically.
Instead, this commit rewrites the `allow_empty()` logic to remove the
over-arching requirement that `allow_empty` be specified in order to
reach any of the keep/drop behaviors. Only if the commit was originally
empty will `allow_empty` have an effect.
For some amount of backwards compatibility with the existing code and
tests, I have opted to preserve the behavior of returning 0 when:
- `allow_empty` is specified, and
- either `is_index_unchanged` or `is_original_commit_empty` indicates an
error
This is primarily out of caution -- I am not positive what downstream
impacts this might have.
Note that this commit is a breaking change: `--keep-redundant-commits`
will no longer imply `--allow-empty`. It would be possible to maintain
the current behavior of `--keep-redundant-commits` implying
`--allow-empty` if it were needed to avoid a breaking change, but I
believe that decoupling them entirely is the correct behavior.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
Disclaimer: This is my first contribution to the git project, and thus
my first attempt at submitting a patch via `git-send-email`. It is also
the first time I've touched worked in C in over a decade, and I really
didn't work with it much before that either. I welcome any and all
feedback on what I may have gotten wrong regarding the patch submission
process, the code changes, or my commit messages.
This is the first in a series of commits that aims to introduce an
`--empty` option to `git-cherry-pick` that provides the same flexibility
as the `--empty` options for `git-rebase` and `git-am`, as well as
improve the consistency in the values and documentation for this option
across the three commands.
The main thing that may be controversial with this particular commit is
that I am proposing a breaking change. As described in the above
message, I do not think that it makes sense to tie `--allow-empty` and
`--keep-redundant-commits` together since they appear to be intended to
work with different types of empty commits. That being said, if it is
deemed unacceptable to make this breaking change, we can consider an
alternative approach where we maintain the behavior of
`--keep-redundant-commits` implying `--allow-empty`, while preventing
the need for the future `--empty=drop` to have that same implication.
Documentation/git-cherry-pick.txt | 10 +++++++---
builtin/revert.c | 4 ----
sequencer.c | 18 ++++++++++--------
t/t3505-cherry-pick-empty.sh | 5 +++++
4 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fdcad3d200..806295a730 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -131,8 +131,8 @@ effect to your index in a row.
even without this option. Note also, that use of this option only
keeps commits that were initially empty (i.e. the commit recorded the
same tree as its parent). Commits which are made empty due to a
- previous commit are dropped. To force the inclusion of those commits
- use `--keep-redundant-commits`.
+ previous commit will cause the cherry-pick to fail. To force the
+ inclusion of those commits use `--keep-redundant-commits`.
--allow-empty-message::
By default, cherry-picking a commit with an empty message will fail.
@@ -144,7 +144,11 @@ effect to your index in a row.
current history, it will become empty. By default these
redundant commits cause `cherry-pick` to stop so the user can
examine the commit. This option overrides that behavior and
- creates an empty commit object. Implies `--allow-empty`.
+ creates an empty commit object. Note that use of this option only
+ results in an empty commit when the commit was not initially empty,
+ but rather became empty due to a previous commit. Commits that were
+ initially empty will cause the cherry-pick to fail. To force the
+ inclusion of those commits use `--allow-empty`.
--strategy=<strategy>::
Use the given merge strategy. Should only be used once.
diff --git a/builtin/revert.c b/builtin/revert.c
index e6f9a1ad26..b2cfde7a87 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -136,10 +136,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
- /* implies allow_empty */
- if (opts->keep_redundant_commits)
- opts->allow_empty = 1;
-
if (cleanup_arg) {
opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
opts->explicit_cleanup = 1;
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..582bde8d46 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
*
* (4) we allow both.
*/
- if (!opts->allow_empty)
- return 0; /* let "git commit" barf as necessary */
-
index_unchanged = is_index_unchanged(r);
- if (index_unchanged < 0)
+ if (index_unchanged < 0) {
+ if (!opts->allow_empty)
+ return 0;
return index_unchanged;
+ }
if (!index_unchanged)
return 0; /* we do not have to say --allow-empty */
- if (opts->keep_redundant_commits)
- return 1;
-
originally_empty = is_original_commit_empty(commit);
- if (originally_empty < 0)
+ if (originally_empty < 0) {
+ if (!opts->allow_empty)
+ return 0;
return originally_empty;
+ }
if (originally_empty)
+ return opts->allow_empty;
+ else if (opts->keep_redundant_commits)
return 1;
else if (opts->drop_redundant_commits)
return 2;
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index eba3c38d5a..6adfd25351 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -59,6 +59,11 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
test_must_fail git cherry-pick empty-change-branch
'
+test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' '
+ git checkout main &&
+ test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch
+'
+
test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
git checkout main &&
git cherry-pick --allow-empty empty-change-branch
--
2.41.0
^ permalink raw reply related
* Re: [PATCH v2 3/4] t7450: test submodule urls
From: Patrick Steinhardt @ 2024-01-19 6:05 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Jeff King, Victoria Dye
In-Reply-To: <b6843a58389170a45b5ef7809e0335a6425eadaa.1705542918.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 960 bytes --]
On Thu, Jan 18, 2024 at 01:55:17AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different
> submodule URLs. To verify this directly (without setting up test
> repositories & submodules), add a 'check-url' subcommand to 'test-tool
> submodule' that calls 'check_submodule_url' in the same way that
> 'check-name' calls 'check_submodule_name'.
>
> Add two tests to separately address cases where the URL check correctly
> filters out invalid URLs and cases where the check misses invalid URLs. Mark
> the latter ("url check misses invalid cases") with 'test_expect_failure' to
> indicate that this not the undesired behavior.
Nit: this should probably say "to indicate that this is not the desired
behaviour." But given that the other patches in this series look good to
me I don't think this warrants a reroll.
Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Elijah Newren @ 2024-01-19 5:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqjzo6m37n.fsf@gitster.g>
On Thu, Jan 18, 2024 at 7:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> Heh, I was hoping that we should be able to use "diff --name-only".
> >>
> >> $ git mv Makefile Breakfile
> >> $ git diff --name-only -M HEAD
> >> Breakfile
> >> $ git reset --hard
> >> $ git rm Makefile
> >> $ >Breakfile && git add Breakfile
> >> $ git diff --name-only -M HEAD
> >> Breakfile
> >> Makefile
> >> $ git reset --hard
> >
> > I guess we could, since the only thing in this repository is a file
> > which is being renamed, and we can then deduce based on the setup that
> > the change we expected must have happened.
> >
> > However, I didn't like the deduce bit; since --name-only only lists
> > one of the two filenames and doesn't provide any hint that the change
> > involved is a rename, it felt to me like using --name-only would make
> > the test not really be a rename test.
>
> Hmph, I am not quite seeing what you are saying. If the "mv" from
> Makefile to Breakfile in the above example is between preimage and
> postimage that are similar enough, then we will see "one" paths,
> i.e. the file in the "after" side of the diff. But if the "mv" from
> Makefile to Breakfile involves too large a content change (like,
> say, going from 3800+ lines to an empty file, in the second example
> above), then because such a change from Makefile to Breakfile is too
> dissimilar, we do not judge it as "renamed" and show "two" paths. I
> do not quite see where we need to "deduce".
You just wrote a very well worded paragraph going through the
reasoning involved to prove that a rename is involved. You reasoned,
or deduced, the necessary conclusion quite well. Sure, it might be a
simple deduction given the knowledge of the test setup, but it's still
a deduction.
But perhaps I can put it another way: You can't just look at the
output of `diff --name-only` and say a rename was involved -- unless
you know the test setup and the previous operations. In fact, how
about a possibly contrived alternate scenario: What if `git mv $1 $2`
had a bug where, after doing the expected work, it did the equivalent
of running `git checkout HEAD -- $1` at the end of its operation.
Then we'd see:
$ <tweak Makefile slightly>
$ git mv Makefile Breakfile
$ git diff --name-only -M HEAD
Breakfile
i.e. we get the same output as without the git-mv bug (note that
Makefile will not be listed since it is unmodified), but with the bug
in git-mv there definitely is no rename involved (since there's no
delete to pair with the addition of Breakfile). As such, we'd still
pass the test despite there being no rename. Sure, the example is
somewhat contrived, but I'm just saying that the --name-only output
doesn't actually test or prove that a rename occurred. And since this
test, and all other tests in this particular testfile, are
specifically about renames, the fact that we aren't specifically
testing for something being renamed feels odd to me.
If you still like `diff --name-only` better anyway, that's fine and
I'll switch it. I'm just stating why it seems suboptimal to me.
^ permalink raw reply
* Re: [PATCH 2/2] t0024: refactor to have single command per line
From: Ghanshyam Thakkar @ 2024-01-19 3:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfryunsd1.fsf@gitster.g>
On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
> > + mkdir untarred &&
> > + (
> > + cd untarred &&
> > + "$TAR" -xf ../test.tar
> > + ) &&
>
> I think we assume "$TAR" is modern enough to know about the "C"
> option (see t/t5004-archive-corner-cases.sh), so
>
> mkdir untarred &&
> "$TAR" Cxf untarred test.tar
>
> without even a subshell may be sufficient.
I suppose '"$TAR" Cxf untarred test.tar' is not a valid syntax on
alpine, since it was breaking CI.
Instead changed it to '"$TAR" xf test.tar -C untarred' in v3, which
is how it's written in t/t5004-archive-corner-cases.sh, which passes
CI.
^ permalink raw reply
* [PATCH v3 2/2] t0024: style fix
From: Ghanshyam Thakkar @ 2024-01-19 3:33 UTC (permalink / raw)
To: git; +Cc: gitster, Ghanshyam Thakkar
In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com>
t0024 has multiple command invocations on a single line, which
goes against the style described in CodingGuidelines, thus fix
that.
Also, use the -C flag to give the destination when using $TAR,
therefore, not requiring a subshell.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index fa4da7c2b3..a7f4de4a43 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -20,7 +20,8 @@ test_expect_success setup '
test_expect_success 'tar archive' '
git archive --format=tar HEAD >test.tar &&
- ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
+ mkdir untarred &&
+ "$TAR" xf test.tar -C untarred &&
test_cmp sample untarred/sample
@@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' '
git archive --format=zip HEAD >test.zip &&
- ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
+ mkdir unzipped &&
+ (
+ cd unzipped &&
+ "$GIT_UNZIP" ../test.zip
+ ) &&
test_cmp sample unzipped/sample
--
2.43.0
^ permalink raw reply related
* [PATCH v3 1/2] t0024: avoid losing exit status to pipes
From: Ghanshyam Thakkar @ 2024-01-19 3:33 UTC (permalink / raw)
To: git; +Cc: gitster, Ghanshyam Thakkar
In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com>
Replace pipe with redirection operator '>' to store the output
to a temporary file after 'git archive' command since the pipe
will swallow the command's exit code and a crash won't
necessarily be noticed.
Also fix an unwanted space after redirection '>' to match the
style described in CodingGuidelines.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index a34de56420..fa4da7c2b3 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -9,7 +9,7 @@ test_expect_success setup '
git config core.autocrlf true &&
- printf "CRLF line ending\r\nAnd another\r\n" > sample &&
+ printf "CRLF line ending\r\nAnd another\r\n" >sample &&
git add sample &&
test_tick &&
@@ -19,8 +19,8 @@ test_expect_success setup '
test_expect_success 'tar archive' '
- git archive --format=tar HEAD |
- ( mkdir untarred && cd untarred && "$TAR" -xf - ) &&
+ git archive --format=tar HEAD >test.tar &&
+ ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
test_cmp sample untarred/sample
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Junio C Hamano @ 2024-01-19 3:06 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <CABPp-BEaYkAPphh06R1HrfD03WTv5uy-2q-T0ZMZaxo9hfXv-g@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> Heh, I was hoping that we should be able to use "diff --name-only".
>>
>> $ git mv Makefile Breakfile
>> $ git diff --name-only -M HEAD
>> Breakfile
>> $ git reset --hard
>> $ git rm Makefile
>> $ >Breakfile && git add Breakfile
>> $ git diff --name-only -M HEAD
>> Breakfile
>> Makefile
>> $ git reset --hard
>
> I guess we could, since the only thing in this repository is a file
> which is being renamed, and we can then deduce based on the setup that
> the change we expected must have happened.
>
> However, I didn't like the deduce bit; since --name-only only lists
> one of the two filenames and doesn't provide any hint that the change
> involved is a rename, it felt to me like using --name-only would make
> the test not really be a rename test.
Hmph, I am not quite seeing what you are saying. If the "mv" from
Makefile to Breakfile in the above example is between preimage and
postimage that are similar enough, then we will see "one" paths,
i.e. the file in the "after" side of the diff. But if the "mv" from
Makefile to Breakfile involves too large a content change (like,
say, going from 3800+ lines to an empty file, in the second example
above), then because such a change from Makefile to Breakfile is too
dissimilar, we do not judge it as "renamed" and show "two" paths. I
do not quite see where we need to "deduce".
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Elijah Newren @ 2024-01-19 2:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sebastian Thiel, Elijah Newren via GitGitGadget, git,
Josh Triplett, Phillip Wood
In-Reply-To: <xmqqwms6qwr3.fsf@gitster.g>
Hi,
On Thu, Jan 18, 2024 at 11:14 AM Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> So, all it boils down to is these two questions.
Thanks for summarizing this.
> * Which one between "'git add .' adds '.config' that users did not
> want to add" and "'git clean -f' removes '.config' together with
> other files" a larger problem to the users, who participate in a
> project that already decided to use the new .gitignore feature to
> mark ".config" as "precious", of older versions of Git that
> predate "precious"?
Accidental "git add ." comes with 3 opportunities to correct the
problem before it becomes permanent: before commiting, after
committing but before pushing, and after publishing for patch review
(where it can even be caught by third parties) but before the
patch/PR/MR is accepted and included. At each stage there's a chance
to go back and correct the problem.
Accidental nuking of a file (via either git clean or git checkout or
git merge or whatever), cannot be reviewed or corrected; it's
immediately too late. And given that we're calling this feature
"precious", that seems a little extra unfortunate.
> * What are projects doing to paths that they want to make
> "precious" with the current system? Do they leave them out of
> ".gitignore" and have them subject to accidental "git add ." to
> protect them from "git clean -f"? Or do they list them in
> ".gitignore" to prevent "git add ." from touching, but leave them
> susceptible to accidental removal by "git clean -f"?
Good questions; I have no answers to these.
However, on a closely related note, in my response to Sebastian I
point out that the '$' syntax permits individual teams to prioritize
avoiding either accidental deletions or accidental adds on a filename
or glob granularity, so if folks are concerned with handling by older
Git versions or are just extra concerned with certain files, they can
optimize accordingly. Sadly, the '#(keep)' syntax does not permit
such prioritization and always treats avoiding accidental adds as the
priority (which, in my opinion, is the less important one to generally
prioritze).
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Elijah Newren @ 2024-01-19 2:37 UTC (permalink / raw)
To: Sebastian Thiel
Cc: Junio C Hamano, Elijah Newren via GitGitGadget, git,
Josh Triplett, Phillip Wood
In-Reply-To: <F214D88E-6837-4EAB-896E-DF8CFC315EE7@icloud.com>
On Thu, Jan 18, 2024 at 1:33 PM Sebastian Thiel
<sebastian.thiel@icloud.com> wrote:
>
> Thanks so much for the analysis, as seeing the problem of choosing
> a syntax from the perspective of its effects when using common commands
> like "git add" and "git clean -f" seems very promising!
>
> When thinking about "git add ." vs "git clean -f" one difference comes to
> mind: "git clean -f" is much less desirable it's fatal. "git add ." on the
> other hand leaves room for correction, even when used with `git commit -a"
> (and with the exception of "git commit -am 'too late'").
"git commit -a" and "git commit -am 'too late'", by themselves, will
only commit changes to already-tracked files. So they wouldn't be
problematic alone.
But perhaps the -a was distracting and you were thinking of "git add .
&& git commit -m whatever". That does remove the chance to correct
before creating a commit, but I don't think it's too bad either. Even
though it skips the chance to catch the problem pre-commit, there's
still time to review & correct before publishing for patch review (or
PR review or MR review or whatever you want to call it). And, even if
published for patch review, it can still be caught & corrected by
those doing patch review as well.
So, I just don't see the "accidental add" problem as being very
severe; there are so many chances to catch and correct it.
> To my mind, in order to support projects with both ".config" and
> ".env.secret" they would have to be given a choice of which syntax
> to use, e.g.
>
> # This file shouldn't accidentally be deleted by `git clean`
> $.config
>
> # These files should never be accidentally tracked
> #(keep)
> .env*
Reminds me of https://www.emacswiki.org/pics/static/TabsSpacesBoth.png
;-)
Besides, if for a specific file or filetype, accidental additions are
more important to protect against than accidental nuking, then can't
folks achieve that by simply using
# Don't let older git versions add the file
.env.secret
# For newer git versions, override the above; treat it as precious
(i.e. don't add AND don't accidentally nuke)
$.env.secret
In contrast, if protection against accidental nuking is more important
for certain files, one can use just the second line without the first.
And, whether you have a file with both lines or just the second line,
newer git versions will protect against both accidental nuking and
accidental adding.
In contrast...
Phillip's syntax provides no way to achieve treating accidental nuking
as more important than accidental adding; it can only handle
protection against accidental adding in older Git versions. And, as I
discussed above, the accidental add problem seems much less severe and
is thus the less important problem to protect against.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Elijah Newren @ 2024-01-19 2:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34v6gswv.fsf@gitster.g>
On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I think the current code makes "-n" take precedence, and ignores
> "-f".
:-(
> Shouldn't it either
>
> (1) error out with "-n and -f cannot be used together", or
> (2) let "-n" and "-f" follow the usual "last one wins" rule?
I believe so.
> The latter may be logically cleaner but it is a change that breaks
> backward compatibility big time in a more dangerous direction, so it
> may not be desirable in practice, with too big a downside for a too
> little gain.
Yeah, I think (1) is the safer option, for now. We could potentially
do (1), then wait a long time, then switch to (2).
^ permalink raw reply
* en/diffcore-delta-final-line-fix (Was: Re: What's cooking in git.git (Jan 2024, #05; Tue, 16))
From: Elijah Newren @ 2024-01-19 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqmst5yply.fsf@gitster.g>
On Tue, Jan 16, 2024 at 2:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> * en/diffcore-delta-final-line-fix (2024-01-11) 1 commit
> - diffcore-delta: avoid ignoring final 'line' of file
>
> Rename detection logic ignored the final line of a file if it is an
> incomplete line.
>
> Expecting a reroll.
> cf. <xmqqedenearc.fsf@gitster.g>
> source: <pull.1637.git.1705006074626.gitgitgadget@gmail.com>
Reroll was provided here:
https://lore.kernel.org/git/pull.1637.v2.git.1705119973690.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Elijah Newren @ 2024-01-19 1:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqil3x69dk.fsf@gitster.g>
On Fri, Jan 12, 2024 at 10:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> I am not very happy with the hardcoded 97. You are already using
> >> the non-standard 10% threshold. If the delta detection that
> >> forgets about the last line is so broken as your proposed log
> >> message noted, shouldn't you be able to construct a sample pair of
> >> preimage and postimage for which the broken version gives so low
> >> similarity to be judged not worth treating as a rename, while the
> >> fixed version gives reasonable similarity to be made into a rename,
> >> by the default threshold? That way, the test only needs to see if
> >> we got a rename (with any similarity) or a delete and an add.
> >
> > Oops, the threshold is entirely unnecessary here; not sure why I
> > didn't remember to take it out (originally used the threshold while
> > testing without the fix to just how low of a similarity git thought
> > these nearly identical files had).
> >
> > Since you don't like the threshold, and since we don't seem to have a
> > summary format that reports on the rename without the percentage, I
> > guess I need to munge the output with sed:
> >
> > sed -e "s/^R[0-9]* /R /" actual >actual.munged &&
>
> Heh, I was hoping that we should be able to use "diff --name-only".
>
> $ git mv Makefile Breakfile
> $ git diff --name-only -M HEAD
> Breakfile
> $ git reset --hard
> $ git rm Makefile
> $ >Breakfile && git add Breakfile
> $ git diff --name-only -M HEAD
> Breakfile
> Makefile
> $ git reset --hard
I guess we could, since the only thing in this repository is a file
which is being renamed, and we can then deduce based on the setup that
the change we expected must have happened.
However, I didn't like the deduce bit; since --name-only only lists
one of the two filenames and doesn't provide any hint that the change
involved is a rename, it felt to me like using --name-only would make
the test not really be a rename test.
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Elijah Newren @ 2024-01-19 1:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ghanshyam Thakkar, Christian Couder, git, johannes.schindelin
In-Reply-To: <xmqqo7dvloiu.fsf@gitster.g>
On Mon, Jan 8, 2024 at 9:32 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
>
> >> Specifically, the commit that introduced the comment never wanted to
> >> honor core.bare in the template. I do not think I has core.bare in
> >> mind when I wrote the comment, but I would have described it as the
> >> same category as the repository format version, i.e. something you
> >> would not want to copy, if I were pressed to clarify back then.
> >
> > Then I suppose this warrants updating the TODO comment in
> > create_default_files(), which currently can be interpreted as this
> > being a unwanted behavior. And also amending the testcases which
> > currently display this as knwon breakage.
>
> I obviously agree with that, after saying that I suspect 0f7443bd
> comes from a misunderstanding ;-).
Sounds fine to me. I have no particular interest in supporting
core.bare from the template; it's just that in order to do other
cleanup, I needed to remove the init_is_bare_repository global
variable (see c2f76965d02 ("init-db: remove unnecessary global
variable", 2023-05-16)). Attempting to remove that global variable
made it _look_ like I was changing the code behavior and breaking it
in the case when core.bare was true in the template. I knew possible
code breakage was what code reviewers would ask about. And my best
reading of the fact that the variable existed plus how the code was
written suggested to me that indeed someone else thought this might be
important to support. So, I left the TODO behind to document that I
wasn't breaking the code with my changes (or even changing behavior at
all), and left some hints for the next reader who came along about
where they might start looking if they thought it was important to
fix.
^ permalink raw reply
* [PATCH v2 2/2] t0024: style fix
From: Ghanshyam Thakkar @ 2024-01-19 1:36 UTC (permalink / raw)
To: git; +Cc: gitster, Ghanshyam Thakkar
In-Reply-To: <20240119013745.2476045-1-shyamthakkar001@gmail.com>
t0024 has multiple command invocations on a single line, which
goes against the style described in CodingGuidelines, thus fix
that.
Also, use the -C flag to give the destination when using $TAR,
therefore, not requiring a subshell.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index fa4da7c2b3..9967a646d1 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -20,7 +20,8 @@ test_expect_success setup '
test_expect_success 'tar archive' '
git archive --format=tar HEAD >test.tar &&
- ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
+ mkdir untarred &&
+ "$TAR" Cxf untarred test.tar &&
test_cmp sample untarred/sample
@@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' '
git archive --format=zip HEAD >test.zip &&
- ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
+ mkdir unzipped &&
+ (
+ cd unzipped &&
+ "$GIT_UNZIP" ../test.zip
+ ) &&
test_cmp sample unzipped/sample
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/2] t0024: avoid losing exit status to pipes
From: Ghanshyam Thakkar @ 2024-01-19 1:36 UTC (permalink / raw)
To: git; +Cc: gitster, Ghanshyam Thakkar
In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com>
Replace pipe with redirection operator '>' to store the output
to a temporary file after 'git archive' command since the pipe
will swallow the command's exit code and a crash won't
necessarily be noticed.
Also fix an unwanted space after redirection '>' to match the
style described in CodingGuidelines.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index a34de56420..fa4da7c2b3 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -9,7 +9,7 @@ test_expect_success setup '
git config core.autocrlf true &&
- printf "CRLF line ending\r\nAnd another\r\n" > sample &&
+ printf "CRLF line ending\r\nAnd another\r\n" >sample &&
git add sample &&
test_tick &&
@@ -19,8 +19,8 @@ test_expect_success setup '
test_expect_success 'tar archive' '
- git archive --format=tar HEAD |
- ( mkdir untarred && cd untarred && "$TAR" -xf - ) &&
+ git archive --format=tar HEAD >test.tar &&
+ ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
test_cmp sample untarred/sample
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 09/10] trailer: move arg handling to interpret-trailers.c
From: Junio C Hamano @ 2024-01-19 1:14 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <8a99d0fca21eca41d62dcd682c8b4ae545633bf7.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/trailer.c b/trailer.c
> index e2d541372a3..0a86e0d5afa 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>
> static char *separators = ":";
>
> +const char *default_separators(void)
> +{
> + return separators;
> +}
This allows API users to peek into the current set of separator
bytes (either the default ":" or specified by the configuration
varaible "trailer.separators"), which is an improvement over
directly exposing the "separators" variable, but in a longer term,
do we need to have some "trailer context" object that holds this
and possibly other global variables like this?
I do not demand such further abstraction in this series, but I'd
prefer to see if we all have shared vision into the future.
Thanks.
^ permalink raw reply
* Re: [PATCH 03/10] trailer: unify trailer formatting machinery
From: Linus Arver @ 2024-01-19 1:12 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqr0ientdb.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> Currently have two functions for formatting trailers exposed in
>> trailer.h:
>>
>> void format_trailers(FILE *outfile, struct list_head *head,
>> const struct process_trailer_options *opts);
>>
>> void format_trailers_from_commit(struct strbuf *out, const char *msg,
>> const struct process_trailer_options *opts);
>>
>> and previously these functions, although similar enough (even taking the
>> same process_trailer_options struct pointer), did not build on each
>> other.
>>
>> Make format_trailers_from_commit() rely on format_trailers(). Teach
>> format_trailers() to process trailers with the additional
>> process_trailer_options fields like opts->key_only which is only used by
>> format_trailers_from_commit() and not builtin/interpret-trailers.c.
>
> Yay. It feels a bit disappointing to see the diffstat and learn
> that we are not deleting substantial number of lines.
Indeed ;)
>> ---
>> builtin/interpret-trailers.c | 5 +-
>> pretty.c | 2 +-
>> ref-filter.c | 2 +-
>> trailer.c | 105 +++++++++++++++++++++++++++++------
>> trailer.h | 21 +++----
>> 5 files changed, 102 insertions(+), 33 deletions(-)
>
>> diff --git a/pretty.c b/pretty.c
>> index cf964b060cd..f0721a5214f 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>> goto trailer_out;
>> }
>> if (*arg == ')') {
>> - format_trailers_from_commit(sb, msg + c->subject_off, &opts);
>> + format_trailers_from_commit(msg + c->subject_off, &opts, sb);
>
> I am curious (read: no objection---merely wondering if there is a
> guiding principle behind the choice of the new order) why this new
> parameter ordering.
Glen Choo told me a while ago [1] that we usually put "out parameters"
at the end, and somehow that stuck with me.
> I suspect it was originally written with a
> strbuf-centric worldview and having sb at the beginning may have
> made sense,
Having gotten more familiar with the strbuf.h functions since this patch
was originally written, I agree.
> but if we are moving them around, wouldn't it make more
> sense to have &opts as the first parameter, as this is primarily a
> "trailers" function? Unsure until I read through to the end, but
> that is my gut reaction.
I simply (mechanically) moved "sb" from the beginning of the parameters
to the end, and didn't think much beyond that adjustment.
Your ordering seems fine to me. Noted for the next reroll, but I will
wait until you're done reviewing the other patches before going with the
new ordering (in case you change your mind).
>> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
>> {
>> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
>> @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
>> strbuf_release(&out);
>> }
>>
>> +void format_trailers(struct list_head *head,
>> + const struct process_trailer_options *opts,
>> + struct strbuf *out)
>> +{
>> + struct list_head *pos;
>> + struct trailer_item *item;
>> + int need_separator = 0;
>> +
>> + list_for_each(pos, head) {
>> + item = list_entry(pos, struct trailer_item, list);
>> + if (item->token) {
>> + char c;
>> + ...
>> + if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>> + if (opts->unfold)
>> + unfold_value(&val);
>> +
>> + if (opts->separator && need_separator)
>> + strbuf_addbuf(out, opts->separator);
>> + if (!opts->value_only)
>> + strbuf_addbuf(out, &tok);
>> + if (!opts->key_only && !opts->value_only) {
>> + if (opts->key_value_separator)
>> + strbuf_addbuf(out, opts->key_value_separator);
>> + else {
>> + c = last_non_space_char(tok.buf);
>> + if (c) {
>> + if (!strchr(separators, c))
>> + strbuf_addf(out, "%c ", separators[0]);
>> + }
>> + }
>
> That's an overly deep nesting. I wonder if a small file-scope
> helper function is in order?
>
> static add_separator(struct process_trailer_options *opts,
> const char *token
> struct strbuf *out)
> {
> if (opts->key_value_separator)
> strbuf_addbuf(out, opts->key_value_separator);
> else
> strbuf_addstr(out, ": ");
> }
>
> Or perhaps inside the context of the loop to go over the list of
> trailer items, one iteration of the list_for_each() loop can become
> one call to a small helper function format_one_trailer() and that
> may make the result easier to follow?
This is actually what I've already done (introducing helper functions to
make this more readable and reduce nesting), but in the larger series
(not in this patch series). I think in this patch I tried to avoid
introducing new functions in order to keep the original "shape" of the
existing refactored functions, including all of the nesting that they
originally had. I wanted to keep the original shape because I thought
that would make review simpler.
> In any case, I didn't see anything glaringly wrong so far in this
> series. Let me keep reading.
>
> Thanks.
Thank you for reviewing!
[1] https://lore.kernel.org/git/kl6l5y5qa34v.fsf@chooglen-macbookpro.roam.corp.google.com/
^ permalink raw reply
* Re: [PATCH 07/10] trailer: spread usage of "trailer_block" language
From: Junio C Hamano @ 2024-01-19 1:03 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <9183990583f9a591c79301a20fa327462bb50cf9.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Deprecate the "trailer_info" struct name and replace it with
> "trailer_block". The main reason is to help readability, because
> "trailer_info" on the surface sounds like it's about a single trailer
> when in reality it is a collection of contiguous lines, at least 25% of
> which are trailers.
Together with the introduction of trailer_block_{start,end}()
accessor functions in the previous step, this sounds like the right
thing to do.
"info" is a word with so low information contents. At least "block"
would hint that we are talking about a block of text that makes up
the trailer attached to a single commit.
^ permalink raw reply
* Re: [PATCH 06/10] trailer: make trailer_info struct private
From: Junio C Hamano @ 2024-01-19 0:58 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <0cbe96421c7bf573e8ddc97b2a0aecc894095399.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> In 13211ae23f (trailer: separate public from internal portion of
> trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
> struct to discourage use by trailer.h API users. However it still left
> open the possibility of external use of trailer_info itself. Now that
> there are no external users of trailer_info, we can make this struct
> private.
Surely, and looking good from a cursory glance. I'll defer to
Christian to comment on this step as an area expert, though.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] t0024: refactor to have single command per line
From: Ghanshyam Thakkar @ 2024-01-19 0:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfryunsd1.fsf@gitster.g>
On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:
> > - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
> > + mkdir untarred &&
> > + (
> > + cd untarred &&
> > + "$TAR" -xf ../test.tar
> > + ) &&
>
> I think we assume "$TAR" is modern enough to know about the "C"
> option (see t/t5004-archive-corner-cases.sh), so
>
> mkdir untarred &&
> "$TAR" Cxf untarred test.tar
>
> without even a subshell may be sufficient.
Will update it in v2.
>
> > @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '
> >
> > git archive --format=zip HEAD >test.zip &&
> >
> > - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
> > + mkdir unzipped &&
> > + (
> > + cd unzipped &&
> > + "$GIT_UNZIP" ../test.zip
> > + ) &&
>
> I do not think we assume "$GIT_UNZIP" to always know about the
> equivalent of "C" (is that "-d exdir"?), so what you wrote is the
> best we can do.
Yeah, "-d exdir" would be the equivalent, but there's no mention of it
in the testcases in t5003 or t5004. So, keeping it as is in v2.
Thanks.
^ permalink raw reply
* Re: [PATCH 05/10] sequencer: use the trailer iterator
From: Junio C Hamano @ 2024-01-19 0:45 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <fd4a9d54d9522973a4c22e43cb1d7964033d4837.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> This patch allows for the removal of "trailer_info_get()" from the
> trailer.h API, which will be in the next patch.
>
> Instead of calling "trailer_info_get()", which is a low-level function
> in the trailers implementation (trailer.c), call
> trailer_iterator_advance(), which was specifically designed for public
> consumption in f0939a0eb1 (trailer: add interface for iterating over
> commit trailers, 2020-09-27).
>
> Avoiding "trailer_info_get()" means we don't have to worry about options
> like "no_divider" (relevant for parsing trailers). We also don't have to
> check for things like "info.trailer_start == info.trailer_end" to see
> whether there were any trailers (instead we can just check to see
> whether the iterator advanced at all).
Nice.
> Also, teach the iterator about non-trailer lines, by adding a new field
> called "raw" to hold both trailer and non-trailer lines. This is
> necessary because a "trailer block" is a list of trailer lines of at
> least 25% trailers (see 146245063e (trailer: allow non-trailers in
> trailer block, 2016-10-21)), such that it may hold non-trailer lines.
OK. This would change behaviour, wouldn't it, in the sense that we
used to yield a non-trailer line from the old iterator but the new
one skips them? Is that something we can demonstrate and protect in
tests (presumably once we conclude these refactoring, we would be
able to call into this machinery from unit-testing framework)?
Thanks.
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/shortlog.c | 7 +++++--
> sequencer.c | 35 +++++++++++++++--------------------
> trailer.c | 20 ++++++++++++--------
> trailer.h | 13 +++++++++++++
> 4 files changed, 45 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 1307ed2b88a..dc8fd5a5532 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
> const char *oneline)
> {
> struct trailer_iterator iter;
> - const char *commit_buffer, *body;
> + const char *commit_buffer, *body, *value;
> struct strbuf ident = STRBUF_INIT;
>
> if (!log->trailers.nr)
> @@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
>
> trailer_iterator_init(&iter, body);
> while (trailer_iterator_advance(&iter)) {
> - const char *value = iter.val.buf;
> + if (!iter.is_trailer)
> + continue;
> +
> + value = iter.val.buf;
>
> if (!string_list_has_string(&log->trailers, iter.key.buf))
> continue;
> diff --git a/sequencer.c b/sequencer.c
> index 3cc88d8a800..d199869cda9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> size_t ignore_footer)
> {
> - struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> - struct trailer_info info;
> - size_t i;
> - int found_sob = 0, found_sob_last = 0;
> - char saved_char;
> -
> - opts.no_divider = 1;
> + struct trailer_iterator iter;
> + size_t i = 0, found_sob = 0;
> + char saved_char = sb->buf[sb->len - ignore_footer];
>
> if (ignore_footer) {
> - saved_char = sb->buf[sb->len - ignore_footer];
> sb->buf[sb->len - ignore_footer] = '\0';
> }
>
> - trailer_info_get(&info, sb->buf, &opts);
> + trailer_iterator_init(&iter, sb->buf);
> + while (trailer_iterator_advance(&iter)) {
> + i++;
> + if (sob &&
> + iter.is_trailer &&
> + !strncmp(iter.raw.buf, sob->buf, sob->len)) {
> + found_sob = i;
> + }
> + }
> + trailer_iterator_release(&iter);
>
> if (ignore_footer)
> sb->buf[sb->len - ignore_footer] = saved_char;
>
> - if (info.trailer_block_start == info.trailer_block_end)
> + if (!i)
> return 0;
>
> - for (i = 0; i < info.trailer_nr; i++)
> - if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> - found_sob = 1;
> - if (i == info.trailer_nr - 1)
> - found_sob_last = 1;
> - }
> -
> - trailer_info_release(&info);
> -
> - if (found_sob_last)
> + if (found_sob == i)
> return 3;
> if (found_sob)
> return 2;
> diff --git a/trailer.c b/trailer.c
> index 132f22b3dd7..593717fd56c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1151,6 +1151,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
> struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> strbuf_init(&iter->key, 0);
> strbuf_init(&iter->val, 0);
> + strbuf_init(&iter->raw, 0);
> opts.no_divider = 1;
> trailer_info_get(&iter->internal.info, msg, &opts);
> iter->internal.cur = 0;
> @@ -1158,17 +1159,19 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
>
> int trailer_iterator_advance(struct trailer_iterator *iter)
> {
> - while (iter->internal.cur < iter->internal.info.trailer_nr) {
> - char *trailer = iter->internal.info.trailers[iter->internal.cur++];
> - int separator_pos = find_separator(trailer, separators);
> -
> - if (separator_pos < 1)
> - continue; /* not a real trailer */
> -
> + char *line;
> + int separator_pos;
> + if (iter->internal.cur < iter->internal.info.trailer_nr) {
> + line = iter->internal.info.trailers[iter->internal.cur++];
> + separator_pos = find_separator(line, separators);
> + iter->is_trailer = (separator_pos > 0);
> +
> + strbuf_reset(&iter->raw);
> + strbuf_addstr(&iter->raw, line);
> strbuf_reset(&iter->key);
> strbuf_reset(&iter->val);
> parse_trailer(&iter->key, &iter->val, NULL,
> - trailer, separator_pos);
> + line, separator_pos);
> unfold_value(&iter->val);
> return 1;
> }
> @@ -1180,4 +1183,5 @@ void trailer_iterator_release(struct trailer_iterator *iter)
> trailer_info_release(&iter->internal.info);
> strbuf_release(&iter->val);
> strbuf_release(&iter->key);
> + strbuf_release(&iter->raw);
> }
> diff --git a/trailer.h b/trailer.h
> index 50f70556302..d50c9fd79b2 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -127,6 +127,19 @@ struct trailer_iterator {
> struct strbuf key;
> struct strbuf val;
>
> + /*
> + * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
> + * key/val pair. This field can contain non-trailer lines because it's
> + * valid for a trailer block to contain such lines (i.e., we only
> + * require 25% of the lines in a trailer block to be trailer lines).
> + */
> + struct strbuf raw;
> +
> + /*
> + * 1 if the raw line was parsed as a separate key/val pair.
> + */
> + int is_trailer;
> +
> /* private */
> struct {
> struct trailer_info info;
^ permalink raw reply
* Re: [PATCH 04/10] trailer: delete obsolete formatting functions
From: Junio C Hamano @ 2024-01-19 0:31 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <8d86461475765ac04ad0ed207e46598eb90a45ba.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 79 -------------------------------------------------------
> 1 file changed, 79 deletions(-)
OK, these unused functions are the reason why 03/10 weren't removing
as many lines as I would have expected from the refactoring.
Looking good.
^ 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