* [PATCH 3/5] apply: update t4012 test suite
From: Philip Peterson via GitGitGadget @ 2024-02-18 7:33 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Emily Shaffer, Philip Peterson,
Philip Peterson
In-Reply-To: <pull.1666.git.git.1708241612.gitgitgadget@gmail.com>
From: Philip Peterson <philip.c.peterson@gmail.com>
The test suite matched against a regex that expected the entire errored
output to end before the additional context added to `git apply`.
Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
t/t4012-diff-binary.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index c64d9d2f405..59defaa37b8 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -68,7 +68,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '
git diff >output &&
sed -e "s/-CIT/xCIT/" <output >broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
- detected=$(cat detected) &&
+ detected=$(head -n 1 detected) &&
detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
@@ -77,7 +77,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '
test_expect_success 'apply detecting corrupt patch correctly' '
git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
- detected=$(cat detected) &&
+ detected=$(head -n 1 detected) &&
detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/5] apply: use new promise structures in git-apply logic as a proving ground
From: Philip Peterson via GitGitGadget @ 2024-02-18 7:33 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Emily Shaffer, Philip Peterson,
Philip Peterson
In-Reply-To: <pull.1666.git.git.1708241612.gitgitgadget@gmail.com>
From: Philip Peterson <philip.c.peterson@gmail.com>
Uses the new promise paradigm in a simple example with git apply and git
am. Several operations that may produce errors are encapsulated in a
promise, so that any errors may be captured and handled according to how
the caller wants to do so.
As an added bonus, we can see more context in error messages now, for
example:
% git apply -3 garbage.patch
error:
could not find header
caused by:
no lines to read
Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
apply.c | 133 +++++++++++++++++++++++++++++++++------------------
apply.h | 9 +++-
range-diff.c | 14 ++++--
3 files changed, 103 insertions(+), 53 deletions(-)
diff --git a/apply.c b/apply.c
index 7608e3301ca..fb6b7074c19 100644
--- a/apply.c
+++ b/apply.c
@@ -26,6 +26,7 @@
#include "object-file.h"
#include "parse-options.h"
#include "path.h"
+#include "promise.h"
#include "quote.h"
#include "read-cache.h"
#include "rerere.h"
@@ -1316,13 +1317,14 @@ static int check_header_line(int linenr, struct patch *patch)
return 0;
}
-int parse_git_diff_header(struct strbuf *root,
+void parse_git_diff_header(struct strbuf *root,
int *linenr,
int p_value,
const char *line,
int len,
unsigned int size,
- struct patch *patch)
+ struct patch *patch,
+ struct promise_t* return_promise)
{
unsigned long offset;
struct gitdiff_data parse_hdr_state;
@@ -1386,10 +1388,12 @@ int parse_git_diff_header(struct strbuf *root,
if (len < oplen || memcmp(p->str, line, oplen))
continue;
res = p->fn(&parse_hdr_state, line + oplen, patch);
- if (res < 0)
- return -1;
- if (check_header_line(*linenr, patch))
- return -1;
+ if (res < 0) {
+ PROMISE_THROW(return_promise, APPLY_ERR_GENERIC, "operation for \"%s\" failed with code: %d", p->str, res);
+ }
+ if (check_header_line(*linenr, patch)) {
+ PROMISE_THROW(return_promise, APPLY_ERR_GENERIC, "invalid header lines");
+ }
if (res > 0)
goto done;
break;
@@ -1399,25 +1403,25 @@ int parse_git_diff_header(struct strbuf *root,
done:
if (!patch->old_name && !patch->new_name) {
if (!patch->def_name) {
- error(Q_("git diff header lacks filename information when removing "
- "%d leading pathname component (line %d)",
- "git diff header lacks filename information when removing "
- "%d leading pathname components (line %d)",
- parse_hdr_state.p_value),
- parse_hdr_state.p_value, *linenr);
- return -128;
+ PROMISE_THROW(return_promise, -128,
+ Q_("git diff header lacks filename information when removing "
+ "%d leading pathname component (line %d)",
+ "git diff header lacks filename information when removing "
+ "%d leading pathname components (line %d)",
+ parse_hdr_state.p_value),
+ parse_hdr_state.p_value, *linenr
+ );
}
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
if ((!patch->new_name && !patch->is_delete) ||
(!patch->old_name && !patch->is_new)) {
- error(_("git diff header lacks filename information "
+ PROMISE_THROW(return_promise, -128, _("git diff header lacks filename information "
"(line %d)"), *linenr);
- return -128;
}
patch->is_toplevel_relative = 1;
- return offset;
+ PROMISE_SUCCEED(return_promise, offset);
}
static int parse_num(const char *line, unsigned long *p)
@@ -1539,16 +1543,17 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra
/*
* Find file diff header
*
- * Returns:
+ * Resolves promise with:
* -1 if no header was found
* -128 in case of error
* the size of the header in bytes (called "offset") otherwise
*/
-static int find_header(struct apply_state *state,
+static void find_header(struct apply_state *state,
const char *line,
unsigned long size,
int *hdrsize,
- struct patch *patch)
+ struct patch *patch,
+ struct promise_t* return_promise)
{
unsigned long offset, len;
@@ -1577,9 +1582,8 @@ static int find_header(struct apply_state *state,
struct fragment dummy;
if (parse_fragment_header(line, len, &dummy) < 0)
continue;
- error(_("patch fragment without header at line %d: %.*s"),
+ PROMISE_THROW(return_promise, -128, _("patch fragment without header at line %d: %.*s"),
state->linenr, (int)len-1, line);
- return -128;
}
if (size < len + 6)
@@ -1590,15 +1594,25 @@ static int find_header(struct apply_state *state,
* or mode change, so we handle that specially
*/
if (!memcmp("diff --git ", line, 11)) {
- int git_hdr_len = parse_git_diff_header(&state->root, &state->linenr,
+ int git_hdr_len;
+ struct promise_t *parse_git_diff_header_promise = promise_init();
+ parse_git_diff_header(&state->root, &state->linenr,
state->p_value, line, len,
- size, patch);
- if (git_hdr_len < 0)
- return -128;
+ size, patch, parse_git_diff_header_promise);
+ promise_assert_finished(parse_git_diff_header_promise);
+
+ if (parse_git_diff_header_promise->state == PROMISE_FAILURE) {
+ PROMISE_BUBBLE_UP(return_promise, parse_git_diff_header_promise,
+ _("could not find file diff header"));
+ }
+
+ git_hdr_len = parse_git_diff_header_promise->result.success_result;
+ promise_release(parse_git_diff_header_promise);
if (git_hdr_len <= len)
continue;
*hdrsize = git_hdr_len;
- return offset;
+ PROMISE_SUCCEED(return_promise, offset);
+ return;
}
/* --- followed by +++ ? */
@@ -1615,13 +1629,14 @@ static int find_header(struct apply_state *state,
continue;
/* Ok, we'll consider it a patch */
- if (parse_traditional_patch(state, line, line+len, patch))
- return -128;
+ if (parse_traditional_patch(state, line, line+len, patch)) {
+ PROMISE_THROW(return_promise, -128, "could not parse traditional patch");
+ }
*hdrsize = len + nextlen;
state->linenr += 2;
- return offset;
+ PROMISE_SUCCEED(return_promise, offset);
}
- return -1;
+ PROMISE_THROW(return_promise, APPLY_ERR_GENERIC, "no lines to read");
}
static void record_ws_error(struct apply_state *state,
@@ -2129,19 +2144,29 @@ static int use_patch(struct apply_state *state, struct patch *p)
* reading after seeing a single patch (i.e. changes to a single file).
* Create fragments (i.e. patch hunks) and hang them to the given patch.
*
- * Returns:
+ * Resolves promise with:
* -1 if no header was found or parse_binary() failed,
* -128 on another error,
* the number of bytes consumed otherwise,
* so that the caller can call us again for the next patch.
*/
-static int parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch)
+static void parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch, struct promise_t* return_promise)
{
- int hdrsize, patchsize;
- int offset = find_header(state, buffer, size, &hdrsize, patch);
+ int hdrsize = -1, patchsize, offset;
+ struct promise_t *find_header_promise = promise_init();
+ find_header(state, buffer, size, &hdrsize, patch, find_header_promise);
+ promise_assert_finished(find_header_promise);
- if (offset < 0)
- return offset;
+ if (find_header_promise->state == PROMISE_FAILURE) {
+ PROMISE_BUBBLE_UP(return_promise, find_header_promise, _("could not find header"));
+ }
+
+ offset = find_header_promise->result.success_result;
+ promise_release(find_header_promise);
+
+ if (offset < 0) {
+ PROMISE_SUCCEED(return_promise, offset);
+ }
prefix_patch(state, patch);
@@ -2159,8 +2184,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
size - offset - hdrsize,
patch);
- if (patchsize < 0)
- return -128;
+ if (patchsize < 0) {
+ PROMISE_THROW(return_promise, -128, _("could not parse patch"));
+ }
if (!patchsize) {
static const char git_binary[] = "GIT binary patch\n";
@@ -2173,8 +2199,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
state->linenr++;
used = parse_binary(state, buffer + hd + llen,
size - hd - llen, patch);
- if (used < 0)
- return -1;
+ if (used < 0) {
+ PROMISE_THROW(return_promise, -1, _("could not parse binary patch"));
+ }
if (used)
patchsize = used + llen;
else
@@ -2205,12 +2232,11 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
*/
if ((state->apply || state->check) &&
(!patch->is_binary && !metadata_changes(patch))) {
- error(_("patch with only garbage at line %d"), state->linenr);
- return -128;
+ PROMISE_THROW(return_promise, -128, _("patch with only garbage at line %d"), state->linenr);
}
}
- return offset + hdrsize + patchsize;
+ PROMISE_SUCCEED(return_promise, offset + hdrsize + patchsize);
}
static void reverse_patches(struct patch *p)
@@ -4755,21 +4781,36 @@ static int apply_patch(struct apply_state *state,
return -128;
offset = 0;
while (offset < buf.len) {
- struct patch *patch;
int nr;
+ struct patch *patch;
+ struct promise_t *parse_chunk_promise;
CALLOC_ARRAY(patch, 1);
patch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF);
patch->recount = !!(options & APPLY_OPT_RECOUNT);
- nr = parse_chunk(state, buf.buf + offset, buf.len - offset, patch);
- if (nr < 0) {
+
+ parse_chunk_promise = promise_init();
+ parse_chunk(state, buf.buf + offset, buf.len - offset, patch, parse_chunk_promise);
+
+ promise_assert_finished(parse_chunk_promise);
+
+ if (parse_chunk_promise->state == PROMISE_FAILURE) {
+ int nr;
+ nr = parse_chunk_promise->result.failure_result.status;
free_patch(patch);
if (nr == -128) {
+ error("\n\t%s", parse_chunk_promise->result.failure_result.message.buf);
+ promise_release(parse_chunk_promise);
res = -128;
goto end;
}
+ promise_release(parse_chunk_promise);
break;
}
+
+ nr = parse_chunk_promise->result.success_result;
+ promise_release(parse_chunk_promise);
+
if (state->apply_in_reverse)
reverse_patches(patch);
if (use_patch(state, patch)) {
diff --git a/apply.h b/apply.h
index 7cd38b1443c..44af75883c5 100644
--- a/apply.h
+++ b/apply.h
@@ -5,6 +5,10 @@
#include "lockfile.h"
#include "string-list.h"
#include "strmap.h"
+#include "promise.h"
+
+/* Error codes (must be less than 0) */
+#define APPLY_ERR_GENERIC -1
struct repository;
@@ -165,13 +169,14 @@ int check_apply_state(struct apply_state *state, int force_apply);
*
* Returns -1 on failure, the length of the parsed header otherwise.
*/
-int parse_git_diff_header(struct strbuf *root,
+void parse_git_diff_header(struct strbuf *root,
int *linenr,
int p_value,
const char *line,
int len,
unsigned int size,
- struct patch *patch);
+ struct patch *patch,
+ struct promise_t *promise);
void release_patch(struct patch *patch);
diff --git a/range-diff.c b/range-diff.c
index c45b6d849cb..3ef8b976a0c 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -121,8 +121,8 @@ static int read_patches(const char *range, struct string_list *list,
if (starts_with(line, "diff --git")) {
struct patch patch = { 0 };
struct strbuf root = STRBUF_INIT;
+ struct promise_t *parse_git_diff_header_promise = promise_init();
int linenr = 0;
- int orig_len;
in_header = 0;
strbuf_addch(&buf, '\n');
@@ -130,16 +130,20 @@ static int read_patches(const char *range, struct string_list *list,
util->diff_offset = buf.len;
if (eol)
*eol = '\n';
- orig_len = len;
- len = parse_git_diff_header(&root, &linenr, 0, line,
- len, size, &patch);
- if (len < 0) {
+ parse_git_diff_header(&root, &linenr, 0, line,
+ len, size, &patch, parse_git_diff_header_promise);
+ promise_assert_finished(parse_git_diff_header_promise);
+ if (parse_git_diff_header_promise->state == PROMISE_FAILURE) {
+ int orig_len = len;
error(_("could not parse git header '%.*s'"),
orig_len, line);
FREE_AND_NULL(util);
string_list_clear(list, 1);
+ promise_release(parse_git_diff_header_promise);
goto cleanup;
}
+ len = parse_git_diff_header_promise->result.success_result;
+ promise_release(parse_git_diff_header_promise);
strbuf_addstr(&buf, " ## ");
if (patch.is_new > 0)
strbuf_addf(&buf, "%s (new)", patch.new_name);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/5] apply: pass through quiet flag to fix t4150
From: Philip Peterson via GitGitGadget @ 2024-02-18 7:33 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Emily Shaffer, Philip Peterson,
Philip Peterson
In-Reply-To: <pull.1666.git.git.1708241612.gitgitgadget@gmail.com>
From: Philip Peterson <philip.c.peterson@gmail.com>
This test was failing because it expects the invocation of `git apply`
to be silent. Because previous patches introduce verbosity where
previously there was a silent error (in the form of a return code), this
adds an opportunity for a bug to become visible. The bug is in the way
`git am` invokes `git apply`, not passing through --quiet when it is
specified.
Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
builtin/am.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/am.c b/builtin/am.c
index d1990d7edcb..799cb8128a3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -36,6 +36,7 @@
#include "mailinfo.h"
#include "apply.h"
#include "string-list.h"
+#include "packfile.h"
#include "pager.h"
#include "path.h"
#include "repository.h"
@@ -2412,6 +2413,10 @@ int cmd_am(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, usage, 0);
+ if (state.quiet) {
+ strvec_push(&state.git_apply_opts, "--quiet");
+ }
+
if (binary >= 0)
fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
"it will be removed. Please do not use it anymore."));
--
gitgitgadget
^ permalink raw reply related
* [PATCH 5/5] am: update test t4254 by adding the new error text
From: Philip Peterson via GitGitGadget @ 2024-02-18 7:33 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Emily Shaffer, Philip Peterson,
Philip Peterson
In-Reply-To: <pull.1666.git.git.1708241612.gitgitgadget@gmail.com>
From: Philip Peterson <philip.c.peterson@gmail.com>
The old error text was shorter than the new, so this just adds the new
context message to the expected text.
Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
t/t4254-am-corrupt.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 45f1d4f95e5..265bcc41bbd 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -60,7 +60,14 @@ test_expect_success setup '
test_expect_success 'try to apply corrupted patch' '
test_when_finished "git am --abort" &&
test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&
- echo "error: git diff header lacks filename information (line 4)" >expected &&
+ space=" " &&
+ echo \
+"error:$space
+ could not find header
+caused by:
+ could not find file diff header
+caused by:
+ git diff header lacks filename information (line 4)" >expected &&
test_path_is_file f &&
test_cmp expected actual
'
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Phillip Wood @ 2024-02-18 10:32 UTC (permalink / raw)
To: MithicSpirit, git
In-Reply-To: <20240218033146.372727-2-rpc01234@gmail.com>
Hi Ricardo
On 18/02/2024 03:30, MithicSpirit wrote:
> Adds options `stash.keepIndex` and `stash.includeUntracked`, which
> enable `--keep-index` and `--include-untracked` by default (unless it
> would conflict with another option). This is done to facilitate the
> workflow where a user:
>
> . Makes modifications;
> . Selectively adds them with `git add -p`; and
> . Stashes the unadded changes to run tests with only the current
> modifications.
>
> `stash.keepIndex` is especially important for this, as otherwise the
> work done in `git add -p` is lost since applying the stash does not
> restore the index.
How does "stash.keepIndex" interact with "git rebase --autostash" and
"git merge --autostash"? I think both those commands expect a clean
index after running "git stash". They could just override the config
setting but it might get a bit confusing if some commands respect the
config and others don't.
> This problem could also be solved by adding
> functionality to the stash to restore the index specifically, but this
> would create much more complexity and still wouldn't be as convenient
> for that workflow.
>
> Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com>
> ---
> This is my first patch to git and my first time using mailing lists for this
> kind of stuff. Please do let me know of any mistakes or gaffes I've made.
I've only given the patch a very quick scan, but it looked sensible. The
only thing that jumped out at me was that quite a few tests seem to do
git init repo &&
(
cd repo &&
# test things
) &&
Our normal practice is to run all the tests in the same file in the same
repository rather than setting up a new one each time.
Best Wishes
Phillip
> Documentation/config/stash.txt | 13 ++++
> builtin/stash.c | 65 ++++++++++++------
> t/t3903-stash.sh | 119 +++++++++++++++++++++++++++++++++
> 3 files changed, 178 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt
> index ec1edaeba6..d6d9ea7daa 100644
> --- a/Documentation/config/stash.txt
> +++ b/Documentation/config/stash.txt
> @@ -1,6 +1,19 @@
> +stash.includeUntracked::
> + Boolean option that configures whether the `git stash push` and `git
> + stash save` commands also stash untracked files by default. This option
> + is ignored if `--include-untracked`, `--no-include-untracked`, `--all`,
> + `--patch`, or `--staged` are used. Defaults to `false`. See
> + linkgit:git-stash[1].
> +
> +stash.keepIndex::
> + Boolean option that configures whether the `git stash push` and `git
> + stash save` commands also stash changes that have been added to the
> + index. This option is ignored if `--keep-index`, `--no-keep-index`, or
> + `--patch` are used. Defaults to `false`. See linkgit:git-stash[1].
> +
> stash.showIncludeUntracked::
> If this is set to true, the `git stash show` command will show
> the untracked files of a stash entry. Defaults to false. See
> the description of the 'show' command in linkgit:git-stash[1].
>
> stash.showPatch::
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 7fb355bff0..c591a2bf4b 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix)
> return run_command(&cp);
> }
>
> static int show_stat = 1;
> static int show_patch;
> static int show_include_untracked;
> +static int default_keep_index;
> +static int default_include_untracked;
>
> static int git_stash_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb)
> {
> if (!strcmp(var, "stash.showstat")) {
> show_stat = git_config_bool(var, value);
> @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value,
> return 0;
> }
> if (!strcmp(var, "stash.showincludeuntracked")) {
> show_include_untracked = git_config_bool(var, value);
> return 0;
> }
> + if (!strcmp(var, "stash.keepindex")) {
> + default_keep_index = git_config_bool(var, value);
> + return 0;
> + }
> + if (!strcmp(var, "stash.includeuntracked")) {
> + default_include_untracked = git_config_bool(var, value);
> + return 0;
> + }
> return git_diff_basic_config(var, value, ctx, cb);
> }
>
> static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt)
> {
> const struct object_id *oid[] = { &info->w_commit, &info->u_tree };
> @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
> int ret = 0;
> struct stash_info info = STASH_INFO_INIT;
> struct strbuf patch = STRBUF_INIT;
> struct strbuf stash_msg_buf = STRBUF_INIT;
> struct strbuf untracked_files = STRBUF_INIT;
>
> - if (patch_mode && keep_index == -1)
> - keep_index = 1;
> -
> - if (patch_mode && include_untracked) {
> - fprintf_ln(stderr, _("Can't use --patch and --include-untracked"
> - " or --all at the same time"));
> - ret = -1;
> - goto done;
> + if (keep_index == -1) {
> + if (patch_mode)
> + keep_index = 1;
> + else
> + keep_index = default_keep_index;
> }
>
> - /* --patch overrides --staged */
> - if (patch_mode)
> + if (patch_mode) {
> + if (include_untracked == -1)
> + include_untracked = 0;
> + else if (include_untracked) {
> + fprintf_ln(stderr,
> + _("Can't use --patch and --include-untracked"
> + " or --all at the same time"));
> + ret = -1;
> + goto done;
> + }
> +
> + /* --patch overrides --staged */
> only_staged = 0;
> -
> - if (only_staged && include_untracked) {
> - fprintf_ln(stderr, _("Can't use --staged and --include-untracked"
> - " or --all at the same time"));
> - ret = -1;
> - goto done;
> }
>
> + if (only_staged) {
> + if (include_untracked == -1)
> + include_untracked = 0;
> + else if (include_untracked) {
> + fprintf_ln(
> + stderr,
> + _("Can't use --staged and --include-untracked"
> + " or --all at the same time"));
> + ret = -1;
> + goto done;
> + }
> + }
> +
> + if (include_untracked == -1)
> + include_untracked = default_include_untracked;
> +
> repo_read_index_preload(the_repository, NULL, 0);
> if (!include_untracked && ps->nr) {
> int i;
> char *ps_matched = xcalloc(ps->nr, 1);
>
> /* TODO: audit for interaction with sparse-index. */
> @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
> fprintf_ln(stderr, _("Cannot remove "
> "worktree changes"));
> ret = -1;
> goto done;
> }
>
> - if (keep_index < 1) {
> + if (!keep_index) {
> struct child_process cp = CHILD_PROCESS_INIT;
>
> cp.git_cmd = 1;
> strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--",
> NULL);
> add_pathspecs(&cp.args, ps);
> @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> int push_assumed)
> {
> int force_assume = 0;
> int keep_index = -1;
> int only_staged = 0;
> int patch_mode = 0;
> - int include_untracked = 0;
> + int include_untracked = -1;
> int quiet = 0;
> int pathspec_file_nul = 0;
> const char *stash_msg = NULL;
> const char *pathspec_from_file = NULL;
> struct pathspec ps;
> struct option options[] = {
> @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix)
>
> static int save_stash(int argc, const char **argv, const char *prefix)
> {
> int keep_index = -1;
> int only_staged = 0;
> int patch_mode = 0;
> - int include_untracked = 0;
> + int include_untracked = -1;
> int quiet = 0;
> int ret = 0;
> const char *stash_msg = NULL;
> struct pathspec ps;
> struct strbuf stash_msg_buf = STRBUF_INIT;
> struct option options[] = {
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 00db82fb24..4ffcca742c 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' '
> EOF
> test_must_fail git stash apply 2>err &&
> test_cmp expect err
> )
> '
>
> +setup_dirty() {
> + echo a >tracked &&
> + echo b >added-modified &&
> + git add tracked added-modified &&
> + git commit -m initial &&
> + echo 1 >>tracked &&
> + echo 2 >>added-modified &&
> + echo c >added-new &&
> + echo d >untracked &&
> + git add added-modified added-new
> +}
> +
> +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' '
> + git init using_opt &&
> + test_when_finished rm -rf using_opt &&
> + (
> + cd using_opt &&
> + setup_dirty &&
> + git stash push &&
> + git stash show -u --patch >../using-opt
> + ) &&
> +
> + test_config stash.includeuntracked true &&
> + git init using_config &&
> + test_when_finished rm -rf using_config &&
> + (
> + cd using_config &&
> + setup_dirty &&
> + git stash push &&
> + git stash show -u --patch >../using-config
> + ) &&
> +
> + test_cmp using-opt using-config
> +'
> +
> +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' '
> + git init no_config &&
> + test_when_finished rm -rf no_config &&
> + (
> + cd no_config &&
> + setup_dirty &&
> + git stash push --no-include-untracked &&
> + git stash show -u --patch >../no-config
> + ) &&
> +
> + test_config stash.includeuntracked true &&
> + git init using_config &&
> + test_when_finished rm -rf using_config &&
> + (
> + cd using_config &&
> + setup_dirty &&
> + git stash push --no-include-untracked &&
> + git stash show -u --patch >../using-config
> + ) &&
> +
> + test_cmp no-config using-config
> +'
> +
> +test_expect_success 'stash.includeuntracked succeeds with --patch' '
> + test_config stash.includeuntracked true &&
> + git stash --patch
> +'
> +
> +test_expect_success 'stash.includeuntracked succeeds with --staged' '
> + test_config stash.includeuntracked true &&
> + git stash --staged
> +'
> +
> +test_expect_success 'stash.keepindex equivalent to --keep-index' '
> + git init using_opt &&
> + test_when_finished rm -rf using_opt &&
> + (
> + cd using_opt &&
> + setup_dirty &&
> + git stash push &&
> + git stash show -u --patch >../using-opt
> + ) &&
> +
> + test_config stash.keepindex true &&
> + git init using_config &&
> + test_when_finished rm -rf using_config &&
> + (
> + cd using_config &&
> + setup_dirty &&
> + git stash push &&
> + git stash show -u --patch >../using-config
> + ) &&
> +
> + test_cmp using-opt using-config
> +'
> +
> +test_expect_success 'stash.keepindex yields to --no-keep-index' '
> + git init no_config &&
> + test_when_finished rm -rf no_config &&
> + (
> + cd no_config &&
> + setup_dirty &&
> + git stash push --no-keep-index &&
> + git stash show -u --patch >../no-config
> + ) &&
> +
> + test_config stash.keepindex true &&
> + git init using_config &&
> + test_when_finished rm -rf using_config &&
> + (
> + cd using_config &&
> + setup_dirty &&
> + git stash push --no-keep-index &&
> + git stash show -u --patch >../using-config
> + ) &&
> +
> + test_cmp no-config using-config
> +'
> +
> +test_expect_success 'stash.keepindex succeeds with --patch' '
> + test_config stash.keepindex true &&
> + git stash --patch
> +'
> +
> test_done
> --
> 2.43.2
>
^ permalink raw reply
* [GSoC] Use unsigned integral type for collection of bits
From: eugenio gigante @ 2024-02-18 11:36 UTC (permalink / raw)
To: git
Hi all,
I was looking around the codebase for some field of a structure that
stores collections of bits with signed int type.
I used this simple grep command to search for it:
$ grep -r -n "\tsigned int" .
> ./diffcore.h:63: signed int is_binary : 2;
The struct in question is "diff_filespec" and Junio commented the
declaration of the field as following:
/* data should be considered "binary"; -1 means "don't know yet" */
So, if I understood it correctly, possible values are:
1 -> 01
2 -> 10
-1 -> 11
On the other, by changing it to unsigned values would be:
1 -> 01
2 -> 10
3 -> 11
I read somewhere that one should always prefer unsigned integral type over
signed integral type for a couple of reasons [1].
These involve operations like Modulus, Shifting and Overflow.
I didn't dig too much into how the field is used and if there are cases in which
the mentioned operations are involved (I would like the community
opinion about this topic before).
Moreover, I don’t know if such a change breaks too much code and if
it’s worth it.
Probably it's not that tragic since the header 'diffcore.h' is only
included in two other files,
but maybe I'm missing something. For sure, various If conditions used
by the function
'diff_filespec_is_binary' inside 'diff.c' would have to be changed.
Besides, it's possible that my grep command is not enough and maybe
more "signed int" can be spotted.
Thanks!
Eugenio Gigante.
P.S. I was insecure about how to send this email since it does not
include a commit.
I decide not to use git-send-email. Hoping I didn't mess up the format.
[1]
https://embeddedgurus.com/stack-overflow/2009/05/signed-versus-unsigned-integers/
^ permalink raw reply
* [PATCH] rerere: fix crash in during clear
From: Marcel Röthke @ 2024-02-18 11:49 UTC (permalink / raw)
To: git; +Cc: Marcel Röthke
When rerere_clear is called, for instance when aborting a rebase, and
the current conflict does not have a pre or postimage recorded git
crashes with a SEGFAULT in has_rerere_resolution when accessing the
status member of struct rerere_dir. This happens because scan_rerere_dir
only allocates the status field in struct rerere_dir when a post or
preimage was found. In some cases a segfault may happen even if a post
or preimage was recorded if it was not for the variant of interest and
the number of the variant that is present is lower than the variant of
interest.
This patch solves this by making sure the status field is large enough
to accommodate for the variant of interest so it can be accesses without
checking if it is large enough.
An alternative solution would be to always check before accessing the
status field, but I think the chosen solution aligns better with the
assumptions made elsewhere in the code.
Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
rerere.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/rerere.c b/rerere.c
index ca7e77ba68..3973ccce37 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct string_list *rr)
buf.buf[hexsz] = '\0';
id = new_rerere_id_hex(buf.buf);
id->variant = variant;
+ /* make sure id->collection->status has enough space
+ * for the variant we are interested in */
+ fit_variant(id->collection, variant);
string_list_insert(rr, path)->util = id;
}
strbuf_release(&buf);
--
2.43.2
^ permalink raw reply related
* Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)
From: Kristoffer Haugsbakk @ 2024-02-18 12:42 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Junio C Hamano, Dragan Simic
In-Reply-To: <d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com>
Here’s a failing test. This fails on top of `master` (3e0d3cd5c7 (Merge branch
'jx/dirstat-parseopt-help', 2024-02-15)).
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
-- >8 --
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 879a6dce601..4f180903486 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -549,4 +549,16 @@ test_expect_success 'moving nested submodules' '
git status
'
+test_expect_success '(TODO title) nonsense move' '
+ test_when_finished git reset --hard HEAD &&
+ git reset --hard HEAD &&
+ mkdir -p a &&
+ mkdir -p b &&
+ >a/a.txt &&
+ git add a/a.txt &&
+ test_must_fail git mv a/a.txt a b &&
+ git status --porcelain >actual &&
+ grep "^A[ ]*a/a.txt$" actual
+'
+
test_done
^ permalink raw reply related
* Re: [PATCH] rerere: fix crash in during clear
From: Kristoffer Haugsbakk @ 2024-02-18 13:02 UTC (permalink / raw)
To: Marcel Röthke; +Cc: git
In-Reply-To: <20240218114936.1121077-1-marcel@roethke.info>
Hi
> rerere: fix crash in during clear
“in during clear”? Did you mean “during clear”?
On Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:
> When rerere_clear is called, for instance when aborting a rebase, and
> the current conflict does not have a pre or postimage recorded git
> crashes with a SEGFAULT in has_rerere_resolution when accessing the
> status member of struct rerere_dir. This happens because scan_rerere_dir
> only allocates the status field in struct rerere_dir when a post or
> preimage was found. In some cases a segfault may happen even if a post
> or preimage was recorded if it was not for the variant of interest and
> the number of the variant that is present is lower than the variant of
> interest.
>
> This patch solves this by making sure the status field is large enough
You can simplify “This patch solves this” to “Solve this”; see
`SubmittingPatches` under “imperative-mood”.
> to accommodate for the variant of interest so it can be accesses without
> checking if it is large enough.
“accessed”
>
> An alternative solution would be to always check before accessing the
> status field, but I think the chosen solution aligns better with the
> assumptions made elsewhere in the code.
>
> Signed-off-by: Marcel Röthke <marcel@roethke.info>
> ---
> rerere.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/rerere.c b/rerere.c
> index ca7e77ba68..3973ccce37 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct
> string_list *rr)
> buf.buf[hexsz] = '\0';
> id = new_rerere_id_hex(buf.buf);
> id->variant = variant;
> + /* make sure id->collection->status has enough space
> + * for the variant we are interested in */
Multi-line comments should have the delimiters on separate lines from
the text. See `CodingGuidelines` under “Multi-line comments”.
> + fit_variant(id->collection, variant);
> string_list_insert(rr, path)->util = id;
> }
> strbuf_release(&buf);
> --
> 2.43.2
^ permalink raw reply
* Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API
From: Bo Anderson @ 2024-02-18 14:48 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Bo Anderson via GitGitGadget, git
In-Reply-To: <CAPig+cR_XYjArdYpU-qm+Wont=yEEXe5hANRyz+YRdhv=UZf=Q@mail.gmail.com>
> On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> I haven't studied the SecItem API, so I can't comment on the meat of
> the patch, but I can make a few generic observations...
Thanks for taking a look!
>> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
>> @@ -3,14 +3,39 @@
>> -__attribute__((format (printf, 1, 2)))
>> +#define ENCODING kCFStringEncodingUTF8
>> +static CFStringRef protocol; /* Stores constant strings - not memory managed */
>> +static CFStringRef host;
>> [...]
>> +
>> +static void clear_credential(void)
>> +{
>> + if (host) {
>> + CFRelease(host);
>> + host = NULL;
>> + }
>> + [...]
>> +}
>> +
>> +__attribute__((format (printf, 1, 2), __noreturn__))
>
> The addition of `__noreturn__` to the `__attribute__` seems unrelated
> to the stated purpose of this patch. As such, it typically would be
> placed in its own patch. If it really is too minor for a separate
> patch, mentioning it in the commit message as a "While at it..." would
> be helpful.
Acknowledged. It is indeed a bit of a nothing change that doesn’t really do much on its own, but when paired with the port variable reorder could potentially make a “minor code cleanup” commit.
>> + va_start(args, allocator);
>> + while ((key = va_arg(args, const void *)) != NULL) {
>> + const void *value;
>> + value = va_arg(args, const void *);
>> + if (value)
>> + CFDictionarySetValue(result, key, value);
>> + }
>> + va_end(args);
>
> A couple related comments...
>
> If va_arg() ever returns NULL for `value`, the next iteration of the
> loop will call va_arg() again, but calling va_arg() again after it has
> already returned NULL is likely undefined behavior. At minimum, I
> would have expected this to be written as:
>
> while (...) {
> ...
> if (!value)
> break;
> CFDictionarySetValue(...);
> }
>
> However, isn't it a programmer error if va_arg() returns NULL for
> `value`? If so, I'd think we'd want to scream loudly about that rather
> than silently ignoring it. So, perhaps something like this:
>
> while (...) {
> ...
> if (!value) {
> fprintf(stderr, "BUG: ...");
> abort();
> }
> CFDictionarySetValue(...);
> }
>
> Or, perhaps just call the existing die() function in this file with a
> suitable "BUG ..." message.
>
In this case it’s by design to accept and check for NULL values as it greatly simplifies the code. Inputs to the credential helpers have various optional fields, such as port and path. It is programmer error to pass NULL to the SecItem API (runtime crash) so in order to simplify having to check each individual field in all of the callers (and probably ditch varargs since you can’t really do dynamic varargs), I check the value here instead. That means you can do something like:
create_dictionary(kCFAllocatorDefault,
kSecAttrServer, host,
kSecAttrPath, path, \
kSecAttrPort, port,
NULL)
And it will only include the key-value pairs that have non-NULL values.
It would indeed be programmer error to not pass key-value pairs, though it is equally programmer error to not have a terminating NULL.
>> + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
>> + if (username_buf)
>> + {
>> + write_item("username", username_buf, strlen(username_buf));
>> return;
>> + }
>
> According to the documentation for CFStringGetCStringPtr(), the
> returned C-string is not newly-allocated, so the caller does not have
> to free it. Therefore, can `username_buf` be declared `const char *`
> rather than `char *` to make it clear to readers that nothing is being
> leaked here? Same comment about the `(char *)` cast.
>
>> + /* If we can't get a CString pointer then
>> + * we need to allocate our own buffer */
>
> Style:
>
> /*
> * Multi-line comments
> * are formatted like this.
> */
>
>> + buffer_len = CFStringGetMaximumSizeForEncoding(
>> + CFStringGetLength(account_ref), ENCODING) + 1;
>> + username_buf = xmalloc(buffer_len);
>> + if (CFStringGetCString(account_ref,
>> + username_buf,
>> + buffer_len,
>> + ENCODING)) {
>> + write_item("username", username_buf, buffer_len - 1);
>> + }
>> + free(username_buf);
>
> Okay, this explains why `username_buf` is declared `char *` rather
> than `const char *`. Typically, when we have a situation in which a
> value may or may not need freeing, we use a `to_free` variable like
> this:
>
> const char *username_buf;
> char *to_free = NULL;
> ...
> username_buf = (const char *)CFStringGetCStringPtr(...);
> if (username_buf) {
> ...
> return;
> }
> ...
> username_buf = to_free = xmalloc(buffer_len);
> if (CFStringGetCString(...))
> ...
> free(to_free);
>
> But that may be overkill for this simple case, and what you have here
> may be "good enough" for anyone already familiar with the API and who
> knows that the `return` after CFStringGetCStringPtr() isn't leaking.
Would it make sense to just have a comment paired with the CFStringGetCStringPtr return explaining why it doesn’t need to be freed there? I’m OK with the to_free variable however if that’s clearer. Idea in my mind was pairing it based on `xmalloc` but I can see why pairing based on variable is clearer.
^ permalink raw reply
* Why does the includeif woks how it does?
From: Dominik von Haller @ 2024-02-18 15:37 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
I have been playing around with the includeif from the .gitconfig. It did not work for me at first, but after some help, I did get it to work.
If you are curious. My Problem and what else was discussed here: https://github.com/git-for-windows/git/issues/4823
Anyway. So, I was trying to access the email property which was set through an includeif config. It did not work because I was in a non git directory. Yes, I do know that the property set in includeif is named gitdir, but it was not obvious to me that you need to be in a git tracked directory for it to work.
I am trying to understand why it must be this way. Why does it not work in non git tracked directories?
I am not sure if I am conveying my Question correctly. Please read the messages in the github issue if the content of this email was confusing.
Best regards
Dominik von Haller
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Ricardo C @ 2024-02-18 17:54 UTC (permalink / raw)
To: phillip.wood, git
In-Reply-To: <99346639-5a36-4c2e-a5d7-035c3c1fda8b@gmail.com>
Hi Phillip,
On 2/18/24 05:32, Phillip Wood wrote:
> How does "stash.keepIndex" interact with "git rebase --autostash" and "git
> merge --autostash"? I think both those commands expect a clean index after
> running "git stash". They could just override the config setting but it might
> get a bit confusing if some commands respect the config and others don't.
Both `git rebase --autostash` and `git merge --autostash` seem to be hardcoded
to clean the index, regardless of the configuration or CLI flags. They do not
use regular `git stash` to do so, but rather `git stash create`. This is
unaffected by my changes, since it follows a different code path and does not
accept `--keep-index` nor `--include-untracked`. I'll add some tests for `git
rebase --autostash` and `git merge --autostash`, just in case this behavior is
changed in the future and causes breakage.
> I've only given the patch a very quick scan, but it looked sensible. The only
> thing that jumped out at me was that quite a few tests seem to do
>
> git init repo &&
> (
> cd repo &&
> # test things
> ) &&
>
> Our normal practice is to run all the tests in the same file in the same
> repository rather than setting up a new one each time.
I was doing this because it makes comparing different commands easier, but
looking through other tests again, it seems like I should be comparing the
outputs to hardcoded files anyway.
Thank you,
Ricardo
^ permalink raw reply
* [PATCH] diff.c: use utf8_strwidth() instead of strlen() for display width
From: Chandra Pratap via GitGitGadget @ 2024-02-18 18:37 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
diff.c: use utf8_strwidth() instead of strlen() for display width
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1668%2FChand-ra%2Fdiff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1668/Chand-ra/diff-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1668
diff.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index ccfa1fca0d0..02d60af6749 100644
--- a/diff.c
+++ b/diff.c
@@ -2712,13 +2712,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
* making the line longer than the maximum width.
*/
- /*
- * NEEDSWORK: line_prefix is often used for "log --graph" output
- * and contains ANSI-colored string. utf8_strnwidth() should be
- * used to correctly count the display width instead of strlen().
- */
if (options->stat_width == -1)
- width = term_columns() - strlen(line_prefix);
+ width = term_columns() - utf8_strwidth(line_prefix);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
base-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API
From: Eric Sunshine @ 2024-02-18 18:39 UTC (permalink / raw)
To: Bo Anderson; +Cc: Bo Anderson via GitGitGadget, git
In-Reply-To: <AFC4D25B-D6ED-4706-A804-CA0183B84604@boanderson.me>
On Sun, Feb 18, 2024 at 9:48 AM Bo Anderson <mail@boanderson.me> wrote:
> > On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> + va_start(args, allocator);
> >> + while ((key = va_arg(args, const void *)) != NULL) {
> >> + const void *value;
> >> + value = va_arg(args, const void *);
> >> + if (value)
> >> + CFDictionarySetValue(result, key, value);
> >> + }
> >> + va_end(args);
> >
> > However, isn't it a programmer error if va_arg() returns NULL for
> > `value`? If so, I'd think we'd want to scream loudly about that rather
> > than silently ignoring it. So, perhaps something like this: [...]
>
> In this case it’s by design to accept and check for NULL values as
> it greatly simplifies the code. Inputs to the credential helpers
> have various optional fields, such as port and path. It is
> programmer error to pass NULL to the SecItem API (runtime crash) so
> in order to simplify having to check each individual field in all of
> the callers (and probably ditch varargs since you can’t really do
> dynamic varargs), I check the value here instead. That means you can
> do something like:
>
> create_dictionary(kCFAllocatorDefault,
> kSecAttrServer, host,
> kSecAttrPath, path, \
> kSecAttrPort, port,
> NULL)
>
> And it will only include the key-value pairs that have non-NULL
> values.
>
> It would indeed be programmer error to not pass key-value pairs,
> though it is equally programmer error to not have a terminating
> NULL.
Okay. I had thought that this check was merely protecting against
programmer error, but the described use-case to avoid passing NULL to
SecItem API makes perfect sense. It might be helpful to future readers
to explain this either as a function-level comment (explaining how to
call the function) or as an in-code comment.
> >> + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
> >> + if (username_buf)
> >> + {
> >> + write_item("username", username_buf, strlen(username_buf));
> >> return;
> >> + }
> >
> > According to the documentation for CFStringGetCStringPtr(), the
> > returned C-string is not newly-allocated, so the caller does not have
> > to free it. Therefore, can `username_buf` be declared `const char *`
> > rather than `char *` to make it clear to readers that nothing is being
> > leaked here? Same comment about the `(char *)` cast.
> >
> >> + buffer_len = CFStringGetMaximumSizeForEncoding(
> >> + CFStringGetLength(account_ref), ENCODING) + 1;
> >> + username_buf = xmalloc(buffer_len);
> >> + if (CFStringGetCString(account_ref,
> >> + username_buf,
> >> + buffer_len,
> >> + ENCODING)) {
> >> + write_item("username", username_buf, buffer_len - 1);
> >> + }
> >> + free(username_buf);
> >
> > Okay, this explains why `username_buf` is declared `char *` rather
> > than `const char *`. Typically, when we have a situation in which a
> > value may or may not need freeing, we use a `to_free` variable like
> > this: [...]
> >
> > But that may be overkill for this simple case, and what you have here
> > may be "good enough" for anyone already familiar with the API and who
> > knows that the `return` after CFStringGetCStringPtr() isn't leaking.
>
> Would it make sense to just have a comment paired with the
> CFStringGetCStringPtr return explaining why it doesn’t need to be
> freed there? I’m OK with the to_free variable however if that’s
> clearer. Idea in my mind was pairing it based on `xmalloc` but I can
> see why pairing based on variable is clearer.
Most likely, anyone working on this code is already familiar with the
CoreFoundation API, thus would understand implicitly that this isn't
leaking. But, yes, a simple comment should be plenty sufficient for
everyone else if you are re-rolling anyhow.
^ permalink raw reply
* Re: [GSoC] Use unsigned integral type for collection of bits
From: Eric Sunshine @ 2024-02-18 19:09 UTC (permalink / raw)
To: eugenio gigante; +Cc: git
In-Reply-To: <CAFJh0PTiN18LcKP6LW0d1u2gkodBD2cOJRBzU_subBaFpzM=CA@mail.gmail.com>
On Sun, Feb 18, 2024 at 6:37 AM eugenio gigante
<giganteeugenio2@gmail.com> wrote:
> I was looking around the codebase for some field of a structure that
> stores collections of bits with signed int type.
>
> > ./diffcore.h:63: signed int is_binary : 2;
>
> The struct in question is "diff_filespec" and Junio commented the
> declaration of the field as following:
>
> /* data should be considered "binary"; -1 means "don't know yet" */
>
> I read somewhere that one should always prefer unsigned integral type over
> signed integral type for a couple of reasons [1].
> These involve operations like Modulus, Shifting and Overflow.
In the context of Git, we want to be using `unsigned` for variables
which are "bags of bits", where each bit indicates some "on" or "off"
property. Very frequently, such variables have "flags" in their names.
So, a possible scenario might be something like this:
#define OP_FOO 0x01
#define OP_BAR 0x02
#define OP_ZAZ 0x04
...
unsigned int flags = OP_FOO | OP_ZAZ;
...
if ((flags & OP_ZAZ))
do_some_zaz();
> I didn't dig too much into how the field is used and if there are cases in which
> the mentioned operations are involved (I would like the community
> opinion about this topic before).
>
> Moreover, I don’t know if such a change breaks too much code and if
> it’s worth it.
>
> but maybe I'm missing something. For sure, various If conditions used
> by the function
>
> 'diff_filespec_is_binary' inside 'diff.c' would have to be changed.
The code in question is not being used as a "bag of bits". Rather,
it's a tristate binary with values "not-set", "true", and "false".
Whereas a typical binary could be represented by a single bit, this
one needs the extra bit to handle the "not-set" case. Moreover, it is
idiomatic in the Git codebase for -1 to represent "not-set", so I
think this code is fine as-is since its meaning is clear to those
familiar with the codebase, thus does not need any changes made to it.
> Besides, it's possible that my grep command is not enough and maybe
> more "signed int" can be spotted.
There are cases in the codebase in which a signed type is being used
as a "bag of bits" instead of the more desirable unsigned type. If you
are interested in making such a fix, you might find some candidates
using a search such as this:
git grep -P '(?<!unsigned )int\s+flags'
For example, it finds this instance in `builtin/add.c`:
static int refresh(int verbose, const struct pathspec *pathspec)
{
int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
...
refresh_index(&the_index, flags, pathspec, seen, ...);
}
Taking a look at `read-cache-ll.h`, we see:
int refresh_index(struct index_state *, unsigned int flags, ...);
So, refresh_index() is correctly expecting an unsigned value for
`flags` but refresh() in `builtin/add.c` has undesirably declared
`flags` as signed.
> P.S. I was insecure about how to send this email since it does not
> include a commit.
> I decide not to use git-send-email. Hoping I didn't mess up the format.
Using your preferred email client for general discussion is fine. Most
people only use git-send-email for sending patches.
^ permalink raw reply
* Re: [PATCH] rerere: fix crash in during clear
From: Marcel Röthke @ 2024-02-18 19:38 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <2359c888-a061-44ed-9d69-2aea9e1d3d80@app.fastmail.com>
On 2024-02-18 14:02:42, Kristoffer Haugsbakk wrote:
> > rerere: fix crash in during clear
>
> “in during clear”? Did you mean “during clear”?
Yes, thanks.
> On Sun, Feb 18, 2024, at 12:49, Marcel Röthke wrote:
> > When rerere_clear is called, for instance when aborting a rebase, and
> > the current conflict does not have a pre or postimage recorded git
> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
> > status member of struct rerere_dir. This happens because scan_rerere_dir
> > only allocates the status field in struct rerere_dir when a post or
> > preimage was found. In some cases a segfault may happen even if a post
> > or preimage was recorded if it was not for the variant of interest and
> > the number of the variant that is present is lower than the variant of
> > interest.
> >
> > This patch solves this by making sure the status field is large enough
>
> You can simplify “This patch solves this” to “Solve this”; see
> `SubmittingPatches` under “imperative-mood”.
done
>
> > to accommodate for the variant of interest so it can be accesses without
> > checking if it is large enough.
>
> “accessed”
>
done
> >
> > An alternative solution would be to always check before accessing the
> > status field, but I think the chosen solution aligns better with the
> > assumptions made elsewhere in the code.
> >
> > Signed-off-by: Marcel Röthke <marcel@roethke.info>
> > ---
> > rerere.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/rerere.c b/rerere.c
> > index ca7e77ba68..3973ccce37 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -219,6 +219,9 @@ static void read_rr(struct repository *r, struct
> > string_list *rr)
> > buf.buf[hexsz] = '\0';
> > id = new_rerere_id_hex(buf.buf);
> > id->variant = variant;
> > + /* make sure id->collection->status has enough space
> > + * for the variant we are interested in */
>
> Multi-line comments should have the delimiters on separate lines from
> the text. See `CodingGuidelines` under “Multi-line comments”.
done
Thank you for your feedback!
^ permalink raw reply
* [PATCH v2] rerere: fix crash during clear
From: Marcel Röthke @ 2024-02-18 19:46 UTC (permalink / raw)
To: git; +Cc: Marcel Röthke
In-Reply-To: <20240218114936.1121077-1-marcel@roethke.info>
When rerere_clear is called, for instance when aborting a rebase, and
the current conflict does not have a pre or postimage recorded git
crashes with a SEGFAULT in has_rerere_resolution when accessing the
status member of struct rerere_dir. This happens because scan_rerere_dir
only allocates the status field in struct rerere_dir when a post or
preimage was found. In some cases a segfault may happen even if a post
or preimage was recorded if it was not for the variant of interest and
the number of the variant that is present is lower than the variant of
interest.
Solve this by making sure the status field is large enough to
accommodate for the variant of interest so it can be accessed without
checking if it is large enough.
An alternative solution would be to always check before accessing the
status field, but I think the chosen solution aligns better with the
assumptions made elsewhere in the code.
Signed-off-by: Marcel Röthke <marcel@roethke.info>
---
Range-diff against v1:
1: 93f982d170 ! 1: 68178298fe rerere: fix crash in during clear
@@ Metadata
Author: Marcel Röthke <marcel@roethke.info>
## Commit message ##
- rerere: fix crash in during clear
+ rerere: fix crash during clear
When rerere_clear is called, for instance when aborting a rebase, and
the current conflict does not have a pre or postimage recorded git
@@ Commit message
the number of the variant that is present is lower than the variant of
interest.
- This patch solves this by making sure the status field is large enough
- to accommodate for the variant of interest so it can be accesses without
+ Solve this by making sure the status field is large enough to
+ accommodate for the variant of interest so it can be accessed without
checking if it is large enough.
An alternative solution would be to always check before accessing the
@@ rerere.c: static void read_rr(struct repository *r, struct string_list *rr)
buf.buf[hexsz] = '\0';
id = new_rerere_id_hex(buf.buf);
id->variant = variant;
-+ /* make sure id->collection->status has enough space
-+ * for the variant we are interested in */
++ /*
++ * make sure id->collection->status has enough space
++ * for the variant we are interested in
++ */
+ fit_variant(id->collection, variant);
string_list_insert(rr, path)->util = id;
}
rerere.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/rerere.c b/rerere.c
index ca7e77ba68..4683d6cbb1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -219,6 +219,11 @@ static void read_rr(struct repository *r, struct string_list *rr)
buf.buf[hexsz] = '\0';
id = new_rerere_id_hex(buf.buf);
id->variant = variant;
+ /*
+ * make sure id->collection->status has enough space
+ * for the variant we are interested in
+ */
+ fit_variant(id->collection, variant);
string_list_insert(rr, path)->util = id;
}
strbuf_release(&buf);
--
2.43.2
^ permalink raw reply related
* [PATCH v2 0/5] Speed up git-notes show
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git
In-Reply-To: <20240205204932.16653-1-maarten.bosmans@vortech.nl>
First time contributor here, trying my first git.git patch series.
BACKGROUND
We have a script that runs a range of build tests for all new commits in the
repository and adds a line to the commit note with the result from the test.
Something along the lines of:
occa-build-jit-gnu-cuda-develop: PASSED (<hostname>, 2024-01-01 00:00:00+01:00)
Pretty useful to quickly check that all commits at least build, not only for
master, but also in progress feature branches. (a passing test suite is
generally only required at the merge point)
PROBLEM
The bash script loops over all remote refs and lists the commits newer than
<N> days ago. For each commit its note is read and grep'ed for an existing
test name to see whether the build test needs to run again. The `git note show`
command that is in this loop nest only takes 14ms to execute, but as it is in
a loop, those times add up.
ANALYSIS
When asked to show a note for a specific commit, git looks up the blob hash
for the note and executes `git show` with that hash. That of course adds
the child process overhead, but also causes the initialization of a lot of
log related configuration, such as for decorations or the mailmap. Simply
outputting the blob directly in the main process reduces the run time by
almost halve.
When looking through the git show implementation for useful stuff that command
does that should also be done when showing a note, I could only find the
`setup_pager()` call and some optional textconv stuff.
The second commit is the main one fixing performance. The others are just
eliminating some overhead I noticed when going through the git notes code.
CHANGES WRT V1
Sharing of the show_blob_object() function from log.c. The intention here is
to have `git notes show` behave the same as `git show` when in the future the
latter might be changed to have more sophisticated behaviour for some blobs,
e.g. launching an image viewer for PNG blobs. Non-blob notes are still not
handled, just like in V1. Current master does handle that, but that is not
deemed a case worth handling.
Maarten Bosmans (5):
log: Move show_blob_object() to log.c
notes: avoid launching a child process to show a note blob
notes: use existing function stream_blob_to_fd
notes: do not clean up right before calling die()
notes: use strbuf_attach to take ownership of the object contents
Makefile | 1 +
builtin/log.c | 39 +++++----------------------------------
builtin/notes.c | 38 +++++++++++---------------------------
log.c | 41 +++++++++++++++++++++++++++++++++++++++++
log.h | 11 +++++++++++
5 files changed, 69 insertions(+), 61 deletions(-)
create mode 100644 log.c
create mode 100644 log.h
--
2.35.3
^ permalink raw reply
* [PATCH v2 1/5] log: Move show_blob_object() to log.c
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git; +Cc: Maarten Bosmans
In-Reply-To: <20240218195938.6253-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
This way it can be used outside of builtin/log.c.
The next commit will make builtin/notes.c use it.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
Makefile | 1 +
builtin/log.c | 39 +++++----------------------------------
log.c | 41 +++++++++++++++++++++++++++++++++++++++++
log.h | 11 +++++++++++
4 files changed, 58 insertions(+), 34 deletions(-)
create mode 100644 log.c
create mode 100644 log.h
diff --git a/Makefile b/Makefile
index 78e874099d..1c19d5c0f3 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@ LIB_OBJS += list-objects-filter-options.o
LIB_OBJS += list-objects-filter.o
LIB_OBJS += list-objects.o
LIB_OBJS += lockfile.o
+LIB_OBJS += log.o
LIB_OBJS += log-tree.o
LIB_OBJS += ls-refs.o
LIB_OBJS += mailinfo.o
diff --git a/builtin/log.c b/builtin/log.c
index db1808d7c1..587a4c374d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -32,7 +32,6 @@
#include "parse-options.h"
#include "line-log.h"
#include "branch.h"
-#include "streaming.h"
#include "version.h"
#include "mailmap.h"
#include "progress.h"
@@ -42,7 +41,7 @@
#include "range-diff.h"
#include "tmp-objdir.h"
#include "tree.h"
-#include "write-or-die.h"
+#include "log.h"
#define MAIL_DEFAULT_WRAP 72
#define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
@@ -653,37 +652,6 @@ static void show_tagger(const char *buf, struct rev_info *rev)
strbuf_release(&out);
}
-static int show_blob_object(const struct object_id *oid, struct rev_info *rev, const char *obj_name)
-{
- struct object_id oidc;
- struct object_context obj_context;
- char *buf;
- unsigned long size;
-
- fflush(rev->diffopt.file);
- if (!rev->diffopt.flags.textconv_set_via_cmdline ||
- !rev->diffopt.flags.allow_textconv)
- return stream_blob_to_fd(1, oid, NULL, 0);
-
- if (get_oid_with_context(the_repository, obj_name,
- GET_OID_RECORD_PATH,
- &oidc, &obj_context))
- die(_("not a valid object name %s"), obj_name);
- if (!obj_context.path ||
- !textconv_object(the_repository, obj_context.path,
- obj_context.mode, &oidc, 1, &buf, &size)) {
- free(obj_context.path);
- return stream_blob_to_fd(1, oid, NULL, 0);
- }
-
- if (!buf)
- die(_("git show %s: bad file"), obj_name);
-
- write_or_die(1, buf, size);
- free(obj_context.path);
- return 0;
-}
-
static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
{
unsigned long size;
@@ -770,7 +738,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)
const char *name = rev.pending.objects[i].name;
switch (o->type) {
case OBJ_BLOB:
- ret = show_blob_object(&o->oid, &rev, name);
+ fflush(rev.diffopt.file);
+ bool do_textconv = rev.diffopt.flags.textconv_set_via_cmdline &&
+ rev.diffopt.flags.allow_textconv;
+ ret = show_blob_object(&o->oid, name, do_textconv);
break;
case OBJ_TAG: {
struct tag *t = (struct tag *)o;
diff --git a/log.c b/log.c
new file mode 100644
index 0000000000..5c77707385
--- /dev/null
+++ b/log.c
@@ -0,0 +1,41 @@
+#include "git-compat-util.h"
+#include "gettext.h"
+#include "diff.h"
+#include "log.h"
+#include "notes.h"
+#include "object-name.h"
+#include "repository.h"
+#include "streaming.h"
+#include "write-or-die.h"
+
+/*
+ * Print blob contents to stdout.
+ */
+int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv)
+{
+ struct object_id oidc;
+ struct object_context obj_context;
+ char *buf;
+ unsigned long size;
+
+ if (!do_textconv)
+ return stream_blob_to_fd(1, oid, NULL, 0);
+
+ if (get_oid_with_context(the_repository, obj_name,
+ GET_OID_RECORD_PATH,
+ &oidc, &obj_context))
+ die(_("not a valid object name %s"), obj_name);
+ if (!obj_context.path ||
+ !textconv_object(the_repository, obj_context.path,
+ obj_context.mode, &oidc, 1, &buf, &size)) {
+ free(obj_context.path);
+ return stream_blob_to_fd(1, oid, NULL, 0);
+ }
+
+ if (!buf)
+ die(_("git show %s: bad file"), obj_name);
+
+ write_or_die(1, buf, size);
+ free(obj_context.path);
+ return 0;
+}
diff --git a/log.h b/log.h
new file mode 100644
index 0000000000..464cca52ff
--- /dev/null
+++ b/log.h
@@ -0,0 +1,11 @@
+#ifndef LOG_H
+#define LOG_H
+
+struct object_id;
+
+/*
+ * Print blob contents to stdout.
+ */
+int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv);
+
+#endif
--
2.35.3
^ permalink raw reply related
* [PATCH v2 3/5] notes: use existing function stream_blob_to_fd
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git; +Cc: Maarten Bosmans
In-Reply-To: <20240218195938.6253-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Use functionality from streaming.c and remove the copy_obj_to_fd()
function local to notes.c.
Streaming the blob to stdout instead of copying through an
intermediate buffer might also be more efficient, but at the
size a typical note is, this is unlikely to matter a lot.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 2a31da6c97..184a92d0c1 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -22,6 +22,7 @@
#include "refs.h"
#include "pager.h"
#include "log.h"
+#include "streaming.h"
#include "run-command.h"
#include "parse-options.h"
#include "string-list.h"
@@ -149,18 +150,6 @@ static int list_each_note(const struct object_id *object_oid,
return 0;
}
-static void copy_obj_to_fd(int fd, const struct object_id *oid)
-{
- unsigned long size;
- enum object_type type;
- char *buf = repo_read_object_file(the_repository, oid, &type, &size);
- if (buf) {
- if (size)
- write_or_die(fd, buf, size);
- free(buf);
- }
-}
-
static void write_commented_object(int fd, const struct object_id *object)
{
struct child_process show = CHILD_PROCESS_INIT;
@@ -205,7 +194,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
if (d->given)
write_or_die(fd, d->buf.buf, d->buf.len);
else if (old_note)
- copy_obj_to_fd(fd, old_note);
+ stream_blob_to_fd(fd, old_note, NULL, 0);
strbuf_addch(&buf, '\n');
strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
--
2.35.3
^ permalink raw reply related
* [PATCH v2 2/5] notes: avoid launching a child process to show a note blob
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git; +Cc: Maarten Bosmans
In-Reply-To: <20240218195938.6253-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Avoid the need to launch a subprocess by calling stream_blob_to_fd
directly. This does not only get rid of the overhead of a separate
child process, but also avoids the initalization of the whole log
machinery that `git show` does. That is needed for example to show
decorated commits and applying the mailmap. For simply displaying
a blob however, the only useful thing show does is enabling the pager.
This locks in the expectation that a note oid refers to a blob, and
not a tree or something else. To still keep the option open that the
blob might not be a text blob and in the future we might handle such
notes differently, the show_blob_object() function is called in order
to have the same behaviour as though `git show <note-oid>` was called.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index caf20fd5bd..2a31da6c97 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,7 +20,8 @@
#include "repository.h"
#include "pretty.h"
#include "refs.h"
-#include "exec-cmd.h"
+#include "pager.h"
+#include "log.h"
#include "run-command.h"
#include "parse-options.h"
#include "string-list.h"
@@ -753,7 +754,7 @@ static int show(int argc, const char **argv, const char *prefix)
struct notes_tree *t;
struct object_id object;
const struct object_id *note;
- int retval;
+ int retval = 0;
struct option options[] = {
OPT_END()
};
@@ -778,8 +779,9 @@ static int show(int argc, const char **argv, const char *prefix)
retval = error(_("no note found for object %s."),
oid_to_hex(&object));
else {
- const char *show_args[3] = {"show", oid_to_hex(note), NULL};
- retval = execv_git_cmd(show_args);
+ setup_pager();
+ if (show_blob_object(note, NULL, false))
+ die(_("object %s is not a blob"), oid_to_hex(note));
}
free_notes(t);
return retval;
--
2.35.3
^ permalink raw reply related
* [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git; +Cc: Maarten Bosmans
In-Reply-To: <20240218195938.6253-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Avoid an extra allocation in the strbuf when pushing the string into it.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 6863935d03..5be24a9c58 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -314,10 +314,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
if (type != OBJ_BLOB)
die(_("cannot read note data from non-blob object '%s'."), arg);
- strbuf_add(&msg->buf, value, len);
- free(value);
+ strbuf_attach(&msg->buf, value, len, len + 1);
- msg->buf.len = len;
ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
d->messages[d->msg_nr - 1] = msg;
msg->stripspace = NO_STRIPSPACE;
@@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
if (!prev_buf)
die(_("unable to read %s"), oid_to_hex(note));
if (size)
- strbuf_add(&buf, prev_buf, size);
+ strbuf_attach(&buf, prev_buf, size, size + 1);
if (d.buf.len && size)
append_separator(&buf);
strbuf_insert(&d.buf, 0, buf.buf, buf.len);
- free(prev_buf);
strbuf_release(&buf);
}
--
2.35.3
^ permalink raw reply related
* [PATCH v2 4/5] notes: do not clean up right before calling die()
From: Maarten Bosmans @ 2024-02-18 19:59 UTC (permalink / raw)
To: git; +Cc: Maarten Bosmans
In-Reply-To: <20240218195938.6253-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
For consistency with the other uses of die() in the same function.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 184a92d0c1..6863935d03 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -311,12 +311,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
die(_("failed to resolve '%s' as a valid ref."), arg);
if (!(value = repo_read_object_file(the_repository, &object, &type, &len)))
die(_("failed to read object '%s'."), arg);
- if (type != OBJ_BLOB) {
- strbuf_release(&msg->buf);
- free(value);
- free(msg);
+ if (type != OBJ_BLOB)
die(_("cannot read note data from non-blob object '%s'."), arg);
- }
strbuf_add(&msg->buf, value, len);
free(value);
--
2.35.3
^ permalink raw reply related
* Re: [PATCH] branch: rework the descriptions of rename and copy operations
From: Dragan Simic @ 2024-02-18 20:38 UTC (permalink / raw)
To: Rubén Justo; +Cc: Junio C Hamano, git
In-Reply-To: <c00f6efe-d1f4-4f2c-99cc-ac7a6d93c9ff@gmail.com>
Hello Ruben,
On 2024-02-17 15:58, Rubén Justo wrote:
> On 16-feb-2024 04:32:10, Dragan Simic wrote:
>> Here's what I think the example from above should eventually look
>> like:
>>
>> 'git branch' (--set-upstream-to=<upstream> | -u <upstream>)
>> [<name>]
>> 'git branch' --unset-upstream [<name>]
>
> Well, my point was not about new terms in this series, but to see if
> the
> idea of reusing an existing one might be of interest.
Good point, ensuring such consistency would be really good.
> I find it interesting to have common terms like "<commit>" "<path>",
> "<envvar>"...
... or "<branch>". :)
> This builds intuition in the user, making it easier to grasp the
> meaning
> of a term, which refers to a similar /thing/, regardless of being used
> in different contexts. And in turn, it can also help to better
> understand the context.
Agreed, consistency is good.
> Side note: My gut tells me that my patch 5aea3955bc (branch:
> clarify <oldbranch> term, 2024-01-06) which was originated by a
> report [1] of a user who found the documentation confusing, would
> have been less likely to happen (the report and consequently my
> patch) if "<branchname>" had been used instead of "<oldbranch>" in
> the documentation. Not because "<branchname>" is a better name,
> but because it is already being used in other commands with a
> clearer meaning of: "if not specified it defaults to the current
> branch". And because of that a user might have be able to fill
> the
> gaps in -m|-c. Of course this is based on my intuition, which I
> know is seriously biased.
>
> [1]
> https://lore.kernel.org/git/pull.1613.git.git.1701303891791.gitgitgadget@gmail.com/
After thinking a bit more about the whole thing, I think that using
"<branch>" instead of "<name>" or "<branchname>" would be our best
choice. It would be simple, direct and clear.
Regarding the branch copy and rename operations and their argument
names, perhaps the following would be a good choice:
--copy [<branch>] <destination>
--move [<branch>] <destination>
It would clearly reflect the nature of the performed operations, while
still using "<branch>" consistently, this time to refer to the source
branch. Using "<destination>" to select the destination name should
be pretty much self-descriptive, if you agree.
Of course, I'll get back to this later in this message.
> And not only can it be of help to the user, but also to developers (or
> reviewers) when documenting new commands or options; because there is
> no need to search for a, maybe new, name if there is one that is
> consolidated.
>
> Perhaps, less names can also improve the lives of translators by making
> it easier to reuse some of the translations.
Perhaps. That should be another example of the long-term benefits
achieved through improved consistency.
>> 'git branch' (-m | -M) [<old>] <new>
>> 'git branch' (-c | -C) [<old>] <new>
>> 'git branch' (-d | -D) [-r] <name>...
>> 'git branch' --edit-description [<name>]
>
> So, to your proposal: it does not make things worse, and it reuses
> terms that are already used elsewhere:
>
> $ for a in '<new>' '<old>' '<name>'; do echo $a $(git grep
> --no-line-number -E "$a" v2.44.0-rc1 -- Documentation/git-*.txt | wc
> -l); done
> <new> 7
> <old> 7
> <name> 147
>
> But based on the idea I've just described, IMHO the user might not
> easily find the similarities in <old> with <name>:
I agree after thinking about the whole thing a bit more.
> And it won't be easy either with <name> and other man pages. For
> example we have:
>
> $ git grep -E 'git checkout.*(new-)?branch'
> Documentation/git-checkout.txt
> Documentation/git-checkout.txt:'git checkout' [-q] [-f] [-m]
> [<branch>]
> Documentation/git-checkout.txt:'git checkout' [-q] [-f] [-m] --detach
> [<branch>]
> Documentation/git-checkout.txt:'git checkout' [-q] [-f] [-m]
> [[-b|-B|--orphan] <new-branch>] [<start-point>]
> Documentation/git-checkout.txt:'git checkout' [<branch>]::
> Documentation/git-checkout.txt:$ git checkout -b <branch> --track
> <remote>/<branch>
> Documentation/git-checkout.txt:'git checkout' -b|-B <new-branch>
> [<start-point>]::
> Documentation/git-checkout.txt:$ git checkout <branch>
> Documentation/git-checkout.txt:'git checkout' --detach [<branch>]::
> Documentation/git-checkout.txt: "git branch" with "-f" followed
> by "git checkout" of that branch;
> Documentation/git-checkout.txt:$ git checkout -b <branch> --track
> <remote>/<branch>
I'd say this solidifies "<branch>" as, hopefully, our best choice,
as already proposed above.
> On the names chosen, the risk of bikesheeding is high and there is
> nothing wrong here, so it is way better to let you work. You have
> taken
> the initiative from Junios's response to my patch, so thank you for
> that.
I hope we'll eventually produce a great git-branch(1) man page together.
After that's completed, I have some more plans for improving git-branch,
by introducing some additional operations.
>> > We don't say that "--edit-description" defaults to the current branch;
>> > It is assumed. Perhaps we can take advantage of that assumption in
>> > -m|-c too.
>>
>> We don't say that yet, :)
>
> Yeah, but maybe we can do it without writing it down :)
Maybe, :) but again, spending a few additional words to describe that
might actually be beneficial. We'll see how it goes.
^ permalink raw reply
* Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
From: M Hickford @ 2024-02-18 20:40 UTC (permalink / raw)
To: gitgitgadget; +Cc: git, mail
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
> git-credential-osxkeychain has largely fallen behind other external
> credential helpers in the features it supports, and hasn't received any
> functional changes since 2013. As it stood, osxkeychain failed seven tests
> in the external credential helper test suite:
>
> not ok 8 - helper (osxkeychain) overwrites on store
> not ok 9 - helper (osxkeychain) can forget host
> not ok 11 - helper (osxkeychain) does not erase a password distinct from input
> not ok 15 - helper (osxkeychain) erases all matching credentials
> not ok 18 - helper (osxkeychain) gets password_expiry_utc
> not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes
> not ok 21 - helper (osxkeychain) gets oauth_refresh_token
>
> After this set of patches, osxkeychain passes all tests in the external
credential helper test suite.
Great work!
Could these tests run as part of macOS CI?
^ 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