* [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 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 1/5] promise: add promise pattern to track success/error from operations
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>
Introduce a promise paradigm. A promise starts off in the pending state,
and represents an asynchronous (or synchronous) action that will
eventually end in either a successful result or a failure result. If a
failure result, an error message may be provided.
This allows us to represent tasks which may fail, while deferring any
control flow actions or error printing that may occur in relation to
said task.
Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
Makefile | 1 +
promise.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
promise.h | 71 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 161 insertions(+)
create mode 100644 promise.c
create mode 100644 promise.h
diff --git a/Makefile b/Makefile
index 78e874099d9..4851eb2d822 100644
--- a/Makefile
+++ b/Makefile
@@ -1109,6 +1109,7 @@ LIB_OBJS += preload-index.o
LIB_OBJS += pretty.o
LIB_OBJS += prio-queue.o
LIB_OBJS += progress.o
+LIB_OBJS += promise.o
LIB_OBJS += promisor-remote.o
LIB_OBJS += prompt.o
LIB_OBJS += protocol.o
diff --git a/promise.c b/promise.c
new file mode 100644
index 00000000000..58ed8b67880
--- /dev/null
+++ b/promise.c
@@ -0,0 +1,89 @@
+/*
+ * Generic implementation of callbacks with await checking.
+ */
+#include "promise.h"
+
+void promise_assert_finished(struct promise_t *p) {
+ if (p->state == PROMISE_UNRESOLVED) {
+ BUG("expected promise to have been resolved/rejected");
+ }
+}
+
+void promise_assert_failure(struct promise_t *p) {
+ if (p->state != PROMISE_FAILURE) {
+ BUG("expected promise to have been rejected");
+ }
+}
+
+void promise_resolve(struct promise_t *p, int status) {
+ if (p->state != PROMISE_UNRESOLVED) {
+ BUG("promise was already resolved/rejected");
+ return;
+ }
+ p->result.success_result = status;
+ p->state = PROMISE_SUCCESS;
+}
+
+void promise_reject(struct promise_t *p, int status, const char* fmt, ...) {
+ va_list args;
+ if (p->state != PROMISE_UNRESOLVED) {
+ BUG("promise was already resolved/rejected");
+ return;
+ }
+ p->result.failure_result.status = status;
+
+ strbuf_init(&p->result.failure_result.message, 0);
+
+ va_start(args, fmt);
+ strbuf_vaddf(&p->result.failure_result.message, fmt, args);
+ va_end(args);
+
+ p->state = PROMISE_FAILURE;
+}
+
+struct promise_t *promise_init(void) {
+ // Promises are allocated on the heap, because they represent potentially long-running tasks,
+ // and a stack-allocated value might not live long enough.
+ struct promise_t *new_promise = xmalloc(sizeof(struct promise_t));
+ struct failure_result_t failure_result;
+
+ new_promise->state = PROMISE_UNRESOLVED;
+ failure_result.status = 0;
+ new_promise->result.failure_result = failure_result;
+
+ return new_promise;
+}
+
+/**
+ * Outputs an error message and size from a failed promise. The error message must be
+ * free()'ed by the caller. Calling this function is not allowed if the promise is not
+ * failed.
+ *
+ * Argument `size` may be omitted by passing in NULL.
+ *
+ * Note that although *error_message is null-terminated, its size may be larger
+ * than the terminated string, and its actual size is indicated by *size.
+ */
+void promise_copy_error(struct promise_t *p, char **error_message, size_t *size) {
+ size_t local_size;
+ promise_assert_failure(p);
+
+ *error_message = strbuf_detach(&p->result.failure_result.message, &local_size);
+ if (size) {
+ *size = local_size;
+ }
+
+ // We are only doing a copy, not a consume, so we need to put the error message back
+ // the way we found it.
+ strbuf_add(&p->result.failure_result.message, *error_message, strlen(*error_message));
+}
+
+/**
+ * Fully deallocates the promise as well as the error message, if any.
+ */
+void promise_release(struct promise_t *p) {
+ if (p->state == PROMISE_FAILURE) {
+ strbuf_release(&p->result.failure_result.message);
+ }
+ free(p);
+}
diff --git a/promise.h b/promise.h
new file mode 100644
index 00000000000..c5500eba986
--- /dev/null
+++ b/promise.h
@@ -0,0 +1,71 @@
+#ifndef PROMISE_H
+#define PROMISE_H
+
+#include "git-compat-util.h"
+#include "strbuf.h"
+
+enum promise_state {
+ PROMISE_UNRESOLVED = 0,
+ PROMISE_SUCCESS = 1,
+ PROMISE_FAILURE = 2,
+};
+
+typedef int success_result_t;
+
+struct failure_result_t {
+ int status;
+ struct strbuf message;
+};
+
+struct promise_t {
+ enum promise_state state;
+ union {
+ success_result_t success_result;
+ struct failure_result_t failure_result;
+ } result;
+};
+
+// Function to assert that a promise has been resolved
+void promise_assert_finished(struct promise_t *p);
+
+// Function to assert that a promise has been rejected
+void promise_assert_failure(struct promise_t *p);
+
+// Function to resolve a promise with a success result
+void promise_resolve(struct promise_t *p, int status);
+
+// Function to reject a promise with a failure result and an optional formatted error message
+void promise_reject(struct promise_t *p, int status, const char* fmt, ...);
+
+// Function to create a new promise
+struct promise_t *promise_init(void);
+
+// Copies the error out of a failed promise
+void promise_copy_error(struct promise_t *promise, char **error_message, size_t *size);
+
+// Fully deallocates the promise
+void promise_release(struct promise_t *promise);
+
+#define PROMISE_SUCCEED(p, errcode) do { \
+ promise_resolve(p, errcode); \
+ return; \
+} while (0)
+
+#define PROMISE_THROW(p, errcode, ...) do { \
+ promise_reject(p, errcode, __VA_ARGS__); \
+ return; \
+} while (0)
+
+#define PROMISE_BUBBLE_UP(dst, src, ...) do { \
+ if (strlen(src->result.failure_result.message.buf) != 0) { \
+ strbuf_insertf(&src->result.failure_result.message, 0, "\n\t"); \
+ strbuf_insertf(&src->result.failure_result.message, 0, _("caused by:")); \
+ strbuf_insertf(&src->result.failure_result.message, 0, "\n"); \
+ strbuf_insertf(&src->result.failure_result.message, 0, __VA_ARGS__); \
+ } \
+ promise_reject(dst, src->result.failure_result.status, "%s", src->result.failure_result.message.buf); \
+ promise_release(src); \
+ return; \
+} while (0)
+
+#endif
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/5] promise: introduce promises to track success or error
From: Philip Peterson via GitGitGadget @ 2024-02-18 7:33 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Emily Shaffer, Philip Peterson
Hello all, this is my first patchset so thank you for being patient with me
as I learn the process and conventions of your very fine project. These
patches are intended as part of the Libification effort, to show how we
could use a Promise structure to help return values from functions.
Problems
========
We seek to make libification easier by establishing a pattern for tracking
whether a function errored in a rich way. Currently, any given function
could immediately die(), or use error() to print directly to the console,
bypassing any relevant verbosity checks. The use of die() currently makes
use of Git as a library inconvenient since it is not graceful.
Additionally, returning using return error(...) (as is commonly done) always
just returns a generic error value, -1, which provides little information.
Approach
========
I solve this problem by splitting the single return value into two return
values: error, and message. However, managing two output variables can
require some coordination, and this coordination can be abstracted away by
use of an existing pattern named Promise.
Promise Concept
===============
A promise is a contract representing "some task" that will eventually
complete. Initially a promise is considered in a pending state. When it
completes, one of two codepaths will eventually be entered: reject, or
resolve. Once resolved or rejected, the promise enters a different state
representing the result. Reject or resolve may only be called once on a
given promise.
Until now, everything I described up to this point is consistent with other
implementations, such as the ECMAScript standard for promises. However, this
implementation departs from the complexity of those promises. In this
implementation, promises are simple and canNOT be chained using .then(...)
and do NOT have any notion of automatic bubbling (via re-entering the
pending state).
Sample output and reproduction
==============================
During an error, we can have richer feedback as to what caused the problem.
% git apply garbage.patch
error:
could not find header
caused by:
patch fragment without header at line 1: @@ -2 +2 @@
To reproduce this output, you can use the following patch (garbage.patch):
@@ -2 +2 @@
Goals
=====
I would love to get feedback on this approach. This patchset is kept small,
so as to serve as a minimal proof of concept. It is intended to abstract to
asynchronous use-cases even though this is only a synchronous one.
Eventually, any top-level function, such as apply_all_patches(...) would
return its output via a promise to make the library interface as clean as
possible, but this patchset does not accomplish this goal. Hopefully it can
provide a direction to go in to achieve that.
Diversion
=========
While building this patchset, I noted a bug that may not have a concrete
repro case in the master branch. The bug is that when invoking git am, it
can call out to git apply, passing many flags but interestingly not the
--quiet flag. I included a fix for this issue in the patchset.
Potential Issue
===============
There is one difficulty with this approach, which is the high level of
repetition in the code required. Tracking which promise is which is its own
source of complexity and may make mistakes more prone to happen. If anyone
has suggestions for how to make the code cleaner, I would love to hear.
Thank you, Philip
Philip Peterson (5):
promise: add promise pattern to track success/error from operations
apply: use new promise structures in git-apply logic as a proving
ground
apply: update t4012 test suite
apply: pass through quiet flag to fix t4150
am: update test t4254 by adding the new error text
Makefile | 1 +
apply.c | 133 +++++++++++++++++++++++++++--------------
apply.h | 9 ++-
builtin/am.c | 5 ++
promise.c | 89 +++++++++++++++++++++++++++
promise.h | 71 ++++++++++++++++++++++
range-diff.c | 14 +++--
t/t4012-diff-binary.sh | 4 +-
t/t4254-am-corrupt.sh | 9 ++-
9 files changed, 279 insertions(+), 56 deletions(-)
create mode 100644 promise.c
create mode 100644 promise.h
base-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1666%2Fphilip-peterson%2Fpeterson%2Femail-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1666/philip-peterson/peterson/email-v1
Pull-Request: https://github.com/git/git/pull/1666
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
From: Eric Sunshine @ 2024-02-18 6:38 UTC (permalink / raw)
To: Bo Anderson via GitGitGadget; +Cc: git, Bo Anderson
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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. [...]
>
> osxkeychain also made use of macOS APIs that had been deprecated since 2014.
> Replacement API was able to be used without regressing the minimum supported
> macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer
> in osxkeychain, 2023-05-01).
Although I'm not familiar with the SecItem API nor with the Git
keychain API, I gave this series a readthrough and left a few minor
comments. Aside from a few very minor style nits, perhaps the only
substantive comment was that patch [1/4] could do a slightly better
job of protecting against future programmer error, but even that is
minor in that it doesn't impact the functionality actually implemented
by the patch, thus may not be worth a reroll.
Overall, despite not being familiar with the APIs in question,
everything I read in the patches made sense and was cleanly
implemented. Nicely done.
^ permalink raw reply
* Re: [PATCH 4/4] osxkeychain: store new attributes
From: Eric Sunshine @ 2024-02-18 6:31 UTC (permalink / raw)
To: Bo Anderson via GitGitGadget; +Cc: git, Bo Anderson
In-Reply-To: <f18435b2189bb08bcdba3b28523db1d4484f66cf.1708212896.git.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)
> and a5c76569e7 (credential: new attribute oauth_refresh_token,
> 2023-04-21) introduced new credential attributes but support was missing
> from git-credential-osxkeychain.
> [...]
> Signed-off-by: Bo Anderson <mail@boanderson.me>
> ---
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -6,10 +6,12 @@
> static CFStringRef host;
> +static CFNumberRef port;
> static CFStringRef path;
> -static CFNumberRef port;
> @@ -17,6 +19,10 @@ static void clear_credential(void)
> + if (port) {
> + CFRelease(port);
> + port = NULL;
> + }
> @@ -29,12 +35,18 @@ static void clear_credential(void)
> - if (port) {
> - CFRelease(port);
> - port = NULL;
> + if (password_expiry_utc) {
> + CFRelease(password_expiry_utc);
> + password_expiry_utc = NULL;
> + }
The relocation of `port` is unrelated to the stated purpose of this
patch. We would normally avoid this sort of "noise" change since it
obscures the "real" changes made by the patch, and would instead place
it in its own patch. That said, it's such a minor issue, I doubt that
it's worth a reroll.
^ permalink raw reply
* Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API
From: Eric Sunshine @ 2024-02-18 6:08 UTC (permalink / raw)
To: Bo Anderson via GitGitGadget; +Cc: git, Bo Anderson
In-Reply-To: <f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.
> The replacement SecItem API however is available as far back as macOS
> 10.6.
>
> While supporting older macOS was perhaps prevously a concern,
> git-credential-osxkeychain already requires a minimum of macOS 10.7
> since 5747c8072b (contrib/credential: avoid fixed-size buffer in
> osxkeychain, 2023-05-01) so using the newer API should not regress the
> range of macOS versions supported.
>
> Adapting to use the newer SecItem API also happens to fix two test
> failures in osxkeychain:
>
> 8 - helper (osxkeychain) overwrites on store
> 9 - helper (osxkeychain) can forget host
>
> The new API is compatible with credentials saved with the older API.
>
> Signed-off-by: Bo Anderson <mail@boanderson.me>
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...
> 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.
> @@ -19,70 +44,135 @@ static void die(const char *err, ...)
> +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
> +{
> + va_list args;
> + const void *key;
> + CFMutableDictionaryRef result;
> +
> + result = CFDictionaryCreateMutable(allocator,
> + 0,
> + &kCFTypeDictionaryKeyCallBacks,
> + &kCFTypeDictionaryValueCallBacks);
> +
> +
Style: one blank line is preferred over two
> + 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.
> +static void find_username_in_item(CFDictionaryRef item)
> {
> + CFStringRef account_ref;
> + char *username_buf;
> + CFIndex buffer_len;
>
> + account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
> + if (!account_ref)
> + {
> + write_item("username", "", 0);
> + return;
> + }
Style: opening brace sticks to the `if` line:
if !(account_ref) {
...
}
Same comment applies to the `if` below.
> + 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.
> +static OSStatus find_internet_password(void)
> {
> + CFDictionaryRef attrs;
> + [...]
>
> + attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
> + kSecReturnAttributes, kCFBooleanTrue,
> + kSecReturnData, kCFBooleanTrue,
> + NULL);
> + result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
> + if (result) {
> + goto out;
> + }
We omit braces when the body is a single statement:
if (result)
goto out;
(Same comment applies to other code in this patch.)
> + data = CFDictionaryGetValue(item, kSecValueData);
> + [...]
> +
> +out:
> + CFRelease(attrs);
Good, `attrs` is released in all cases.
> +static OSStatus add_internet_password(void)
> {
> + [...]
> + attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
> + NULL);
> + result = SecItemAdd(attrs, NULL);
> + if (result == errSecDuplicateItem) {
> + CFDictionaryRef query;
> + query = CREATE_SEC_ATTRIBUTES(NULL);
> + result = SecItemUpdate(query, attrs);
> + CFRelease(query);
> + }
> + CFRelease(attrs);
> + return result;
> }
Good, `attrs` and `query` are released in all cases.
^ permalink raw reply
* [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: MithicSpirit @ 2024-02-18 3:30 UTC (permalink / raw)
To: git; +Cc: MithicSpirit
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. 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.
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 related
* [PATCH] doc: remove outdated information about interactive.singleKey
From: Julio Bacellari @ 2024-02-18 3:02 UTC (permalink / raw)
To: git; +Cc: Julio Bacellari
The Perl implementation of add --interactive was removed in commit [1].
Additionally, the interactive.singleKey setting is no longer silently
ignored. The internal implementation of ReadKey [2] displays a warning
if the platform is unsupported.
[1] 20b813d7d (add: remove "add.interactive.useBuiltin" & Perl "git add--interactive", 2023-02-06)
[2] a5e46e6b0 (terminal: add a new function to read a single keystroke, 2020-01-14)
Signed-off-by: Julio Bacellari <julio.bacel@gmail.com>
---
Documentation/config/interactive.txt | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/Documentation/config/interactive.txt b/Documentation/config/interactive.txt
index a2d3c7ec449e..5cc26555f19a 100644
--- a/Documentation/config/interactive.txt
+++ b/Documentation/config/interactive.txt
@@ -4,9 +4,7 @@ interactive.singleKey::
Currently this is used by the `--patch` mode of
linkgit:git-add[1], linkgit:git-checkout[1],
linkgit:git-restore[1], linkgit:git-commit[1],
- linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
- setting is silently ignored if portable keystroke input
- is not available; requires the Perl module Term::ReadKey.
+ linkgit:git-reset[1], and linkgit:git-stash[1].
interactive.diffFilter::
When an interactive command (such as `git add --patch`) shows
--
2.43.0
^ permalink raw reply related
* [PATCH 4/4] osxkeychain: store new attributes
From: Bo Anderson via GitGitGadget @ 2024-02-17 23:34 UTC (permalink / raw)
To: git; +Cc: Bo Anderson, Bo Anderson
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
From: Bo Anderson <mail@boanderson.me>
d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token,
2023-04-21) introduced new credential attributes but support was missing
from git-credential-osxkeychain.
Support these attributes by appending the data to the password in the
keychain, separated by line breaks. Line breaks cannot appear in a git
credential password so it is an appropriate separator.
Fixes the remaining test failures with osxkeychain:
18 - helper (osxkeychain) gets password_expiry_utc
19 - helper (osxkeychain) overwrites when password_expiry_utc
changes
21 - helper (osxkeychain) gets oauth_refresh_token
Signed-off-by: Bo Anderson <mail@boanderson.me>
---
.../osxkeychain/git-credential-osxkeychain.c | 68 +++++++++++++++++--
1 file changed, 62 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 9e742796336..6a40917b1ef 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -6,10 +6,12 @@
#define ENCODING kCFStringEncodingUTF8
static CFStringRef protocol; /* Stores constant strings - not memory managed */
static CFStringRef host;
+static CFNumberRef port;
static CFStringRef path;
static CFStringRef username;
static CFDataRef password;
-static CFNumberRef port;
+static CFDataRef password_expiry_utc;
+static CFDataRef oauth_refresh_token;
static void clear_credential(void)
{
@@ -17,6 +19,10 @@ static void clear_credential(void)
CFRelease(host);
host = NULL;
}
+ if (port) {
+ CFRelease(port);
+ port = NULL;
+ }
if (path) {
CFRelease(path);
path = NULL;
@@ -29,12 +35,18 @@ static void clear_credential(void)
CFRelease(password);
password = NULL;
}
- if (port) {
- CFRelease(port);
- port = NULL;
+ if (password_expiry_utc) {
+ CFRelease(password_expiry_utc);
+ password_expiry_utc = NULL;
+ }
+ if (oauth_refresh_token) {
+ CFRelease(oauth_refresh_token);
+ oauth_refresh_token = NULL;
}
}
+#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
+
__attribute__((format (printf, 1, 2), __noreturn__))
static void die(const char *err, ...)
{
@@ -197,9 +209,27 @@ static OSStatus delete_ref(const void *itemRef)
CFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue);
result = SecItemCopyMatching(query, (CFTypeRef *)&data);
if (!result) {
- if (CFEqual(data, password))
+ CFDataRef kc_password;
+ const UInt8 *raw_data;
+ const UInt8 *line;
+
+ /* Don't match appended metadata */
+ raw_data = CFDataGetBytePtr(data);
+ line = memchr(raw_data, '\n', CFDataGetLength(data));
+ if (line)
+ kc_password = CFDataCreateWithBytesNoCopy(
+ kCFAllocatorDefault,
+ raw_data,
+ line - raw_data,
+ kCFAllocatorNull);
+ else
+ kc_password = data;
+
+ if (CFEqual(kc_password, password))
result = SecItemDelete(delete_query);
+ if (line)
+ CFRelease(kc_password);
CFRelease(data);
}
@@ -250,6 +280,7 @@ static OSStatus delete_internet_password(void)
static OSStatus add_internet_password(void)
{
+ CFMutableDataRef data;
CFDictionaryRef attrs;
OSStatus result;
@@ -257,7 +288,23 @@ static OSStatus add_internet_password(void)
if (!protocol || !host || !username || !password)
return -1;
- attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+ data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
+ if (password_expiry_utc) {
+ CFDataAppendBytes(data,
+ (const UInt8 *)STRING_WITH_LENGTH("\npassword_expiry_utc="));
+ CFDataAppendBytes(data,
+ CFDataGetBytePtr(password_expiry_utc),
+ CFDataGetLength(password_expiry_utc));
+ }
+ if (oauth_refresh_token) {
+ CFDataAppendBytes(data,
+ (const UInt8 *)STRING_WITH_LENGTH("\noauth_refresh_token="));
+ CFDataAppendBytes(data,
+ CFDataGetBytePtr(oauth_refresh_token),
+ CFDataGetLength(oauth_refresh_token));
+ }
+
+ attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, data,
NULL);
result = SecItemAdd(attrs, NULL);
@@ -268,6 +315,7 @@ static OSStatus add_internet_password(void)
CFRelease(query);
}
+ CFRelease(data);
CFRelease(attrs);
return result;
@@ -339,6 +387,14 @@ static void read_credential(void)
password = CFDataCreate(kCFAllocatorDefault,
(UInt8 *)v,
strlen(v));
+ else if (!strcmp(buf, "password_expiry_utc"))
+ password_expiry_utc = CFDataCreate(kCFAllocatorDefault,
+ (UInt8 *)v,
+ strlen(v));
+ else if (!strcmp(buf, "oauth_refresh_token"))
+ oauth_refresh_token = CFDataCreate(kCFAllocatorDefault,
+ (UInt8 *)v,
+ strlen(v));
/*
* Ignore other lines; we don't know what they mean, but
* this future-proofs us when later versions of git do
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] osxkeychain: erase matching passwords only
From: Bo Anderson via GitGitGadget @ 2024-02-17 23:34 UTC (permalink / raw)
To: git; +Cc: Bo Anderson, Bo Anderson
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
From: Bo Anderson <mail@boanderson.me>
Other credential helpers support deleting credentials that match a
specified password. See 7144dee3ec (credential/libsecret: erase matching
creds only, 2023-07-26) and cb626f8e5c (credential/wincred: erase
matching creds only, 2023-07-26).
Support this in osxkeychain too by extracting, decrypting and comparing
the stored password before deleting.
Fixes the following test failure with osxkeychain:
11 - helper (osxkeychain) does not erase a password distinct from
input
Signed-off-by: Bo Anderson <mail@boanderson.me>
---
.../osxkeychain/git-credential-osxkeychain.c | 56 ++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index e9cee3aed45..9e742796336 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -169,9 +169,55 @@ static OSStatus find_internet_password(void)
return result;
}
+static OSStatus delete_ref(const void *itemRef)
+{
+ CFArrayRef item_ref_list;
+ CFDictionaryRef delete_query;
+ OSStatus result;
+
+ item_ref_list = CFArrayCreate(kCFAllocatorDefault,
+ &itemRef,
+ 1,
+ &kCFTypeArrayCallBacks);
+ delete_query = create_dictionary(kCFAllocatorDefault,
+ kSecClass, kSecClassInternetPassword,
+ kSecMatchItemList, item_ref_list,
+ NULL);
+
+ if (password) {
+ /* We only want to delete items with a matching password */
+ CFIndex capacity;
+ CFMutableDictionaryRef query;
+ CFDataRef data;
+
+ capacity = CFDictionaryGetCount(delete_query) + 1;
+ query = CFDictionaryCreateMutableCopy(kCFAllocatorDefault,
+ capacity,
+ delete_query);
+ CFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue);
+ result = SecItemCopyMatching(query, (CFTypeRef *)&data);
+ if (!result) {
+ if (CFEqual(data, password))
+ result = SecItemDelete(delete_query);
+
+ CFRelease(data);
+ }
+
+ CFRelease(query);
+ } else {
+ result = SecItemDelete(delete_query);
+ }
+
+ CFRelease(delete_query);
+ CFRelease(item_ref_list);
+
+ return result;
+}
+
static OSStatus delete_internet_password(void)
{
CFDictionaryRef attrs;
+ CFArrayRef refs;
OSStatus result;
/*
@@ -183,10 +229,18 @@ static OSStatus delete_internet_password(void)
return -1;
attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll,
+ kSecReturnRef, kCFBooleanTrue,
NULL);
- result = SecItemDelete(attrs);
+ result = SecItemCopyMatching(attrs, (CFTypeRef *)&refs);
CFRelease(attrs);
+ if (!result) {
+ for (CFIndex i = 0; !result && i < CFArrayGetCount(refs); i++)
+ result = delete_ref(CFArrayGetValueAtIndex(refs, i));
+
+ CFRelease(refs);
+ }
+
/* We consider not found to not be an error */
if (result == errSecItemNotFound)
result = errSecSuccess;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] osxkeychain: erase all matching credentials
From: Bo Anderson via GitGitGadget @ 2024-02-17 23:34 UTC (permalink / raw)
To: git; +Cc: Bo Anderson, Bo Anderson
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
From: Bo Anderson <mail@boanderson.me>
Other credential managers erased all matching credentials, as indicated
by a test case that osxkeychain failed:
15 - helper (osxkeychain) erases all matching credentials
Signed-off-by: Bo Anderson <mail@boanderson.me>
---
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index dc294ae944a..e9cee3aed45 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -182,7 +182,8 @@ static OSStatus delete_internet_password(void)
if (!protocol || !host)
return -1;
- attrs = CREATE_SEC_ATTRIBUTES(NULL);
+ attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll,
+ NULL);
result = SecItemDelete(attrs);
CFRelease(attrs);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API
From: Bo Anderson via GitGitGadget @ 2024-02-17 23:34 UTC (permalink / raw)
To: git; +Cc: Bo Anderson, Bo Anderson
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>
From: Bo Anderson <mail@boanderson.me>
The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.
The replacement SecItem API however is available as far back as macOS
10.6.
While supporting older macOS was perhaps prevously a concern,
git-credential-osxkeychain already requires a minimum of macOS 10.7
since 5747c8072b (contrib/credential: avoid fixed-size buffer in
osxkeychain, 2023-05-01) so using the newer API should not regress the
range of macOS versions supported.
Adapting to use the newer SecItem API also happens to fix two test
failures in osxkeychain:
8 - helper (osxkeychain) overwrites on store
9 - helper (osxkeychain) can forget host
The new API is compatible with credentials saved with the older API.
Signed-off-by: Bo Anderson <mail@boanderson.me>
---
contrib/credential/osxkeychain/Makefile | 3 +-
.../osxkeychain/git-credential-osxkeychain.c | 265 +++++++++++++-----
2 files changed, 199 insertions(+), 69 deletions(-)
diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 4b3a08a2bac..238f5f8c36f 100644
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall
-include ../../../config.mak
git-credential-osxkeychain: git-credential-osxkeychain.o
- $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security
+ $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \
+ -framework Security -framework CoreFoundation
git-credential-osxkeychain.o: git-credential-osxkeychain.c
$(CC) -c $(CFLAGS) $<
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 5f2e5f16c88..dc294ae944a 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -3,14 +3,39 @@
#include <stdlib.h>
#include <Security/Security.h>
-static SecProtocolType protocol;
-static char *host;
-static char *path;
-static char *username;
-static char *password;
-static UInt16 port;
-
-__attribute__((format (printf, 1, 2)))
+#define ENCODING kCFStringEncodingUTF8
+static CFStringRef protocol; /* Stores constant strings - not memory managed */
+static CFStringRef host;
+static CFStringRef path;
+static CFStringRef username;
+static CFDataRef password;
+static CFNumberRef port;
+
+static void clear_credential(void)
+{
+ if (host) {
+ CFRelease(host);
+ host = NULL;
+ }
+ if (path) {
+ CFRelease(path);
+ path = NULL;
+ }
+ if (username) {
+ CFRelease(username);
+ username = NULL;
+ }
+ if (password) {
+ CFRelease(password);
+ password = NULL;
+ }
+ if (port) {
+ CFRelease(port);
+ port = NULL;
+ }
+}
+
+__attribute__((format (printf, 1, 2), __noreturn__))
static void die(const char *err, ...)
{
char msg[4096];
@@ -19,70 +44,135 @@ static void die(const char *err, ...)
vsnprintf(msg, sizeof(msg), err, params);
fprintf(stderr, "%s\n", msg);
va_end(params);
+ clear_credential();
exit(1);
}
-static void *xstrdup(const char *s1)
+static void *xmalloc(size_t len)
{
- void *ret = strdup(s1);
+ void *ret = malloc(len);
if (!ret)
die("Out of memory");
return ret;
}
-#define KEYCHAIN_ITEM(x) (x ? strlen(x) : 0), x
-#define KEYCHAIN_ARGS \
- NULL, /* default keychain */ \
- KEYCHAIN_ITEM(host), \
- 0, NULL, /* account domain */ \
- KEYCHAIN_ITEM(username), \
- KEYCHAIN_ITEM(path), \
- port, \
- protocol, \
- kSecAuthenticationTypeDefault
-
-static void write_item(const char *what, const char *buf, int len)
+static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
+{
+ va_list args;
+ const void *key;
+ CFMutableDictionaryRef result;
+
+ result = CFDictionaryCreateMutable(allocator,
+ 0,
+ &kCFTypeDictionaryKeyCallBacks,
+ &kCFTypeDictionaryValueCallBacks);
+
+
+ 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);
+
+ return result;
+}
+
+#define CREATE_SEC_ATTRIBUTES(...) \
+ create_dictionary(kCFAllocatorDefault, \
+ kSecClass, kSecClassInternetPassword, \
+ kSecAttrServer, host, \
+ kSecAttrAccount, username, \
+ kSecAttrPath, path, \
+ kSecAttrPort, port, \
+ kSecAttrProtocol, protocol, \
+ kSecAttrAuthenticationType, \
+ kSecAttrAuthenticationTypeDefault, \
+ __VA_ARGS__);
+
+static void write_item(const char *what, const char *buf, size_t len)
{
printf("%s=", what);
fwrite(buf, 1, len, stdout);
putchar('\n');
}
-static void find_username_in_item(SecKeychainItemRef item)
+static void find_username_in_item(CFDictionaryRef item)
{
- SecKeychainAttributeList list;
- SecKeychainAttribute attr;
+ CFStringRef account_ref;
+ char *username_buf;
+ CFIndex buffer_len;
- list.count = 1;
- list.attr = &attr;
- attr.tag = kSecAccountItemAttr;
+ account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
+ if (!account_ref)
+ {
+ write_item("username", "", 0);
+ return;
+ }
- if (SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL))
+ username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
+ if (username_buf)
+ {
+ write_item("username", username_buf, strlen(username_buf));
return;
+ }
- write_item("username", attr.data, attr.length);
- SecKeychainItemFreeContent(&list, NULL);
+ /* If we can't get a CString pointer then
+ * we need to allocate our own buffer */
+ 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);
}
-static void find_internet_password(void)
+static OSStatus find_internet_password(void)
{
- void *buf;
- UInt32 len;
- SecKeychainItemRef item;
+ CFDictionaryRef attrs;
+ CFDictionaryRef item;
+ CFDataRef data;
+ OSStatus result;
- if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
- return;
+ attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
+ kSecReturnAttributes, kCFBooleanTrue,
+ kSecReturnData, kCFBooleanTrue,
+ NULL);
+ result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
+ if (result) {
+ goto out;
+ }
- write_item("password", buf, len);
+ data = CFDictionaryGetValue(item, kSecValueData);
+
+ write_item("password",
+ (const char *)CFDataGetBytePtr(data),
+ CFDataGetLength(data));
if (!username)
find_username_in_item(item);
- SecKeychainItemFreeContent(NULL, buf);
+ CFRelease(item);
+
+out:
+ CFRelease(attrs);
+
+ /* We consider not found to not be an error */
+ if (result == errSecItemNotFound)
+ result = errSecSuccess;
+
+ return result;
}
-static void delete_internet_password(void)
+static OSStatus delete_internet_password(void)
{
- SecKeychainItemRef item;
+ CFDictionaryRef attrs;
+ OSStatus result;
/*
* Require at least a protocol and host for removal, which is what git
@@ -90,25 +180,42 @@ static void delete_internet_password(void)
* Keychain manager.
*/
if (!protocol || !host)
- return;
+ return -1;
- if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, 0, NULL, &item))
- return;
+ attrs = CREATE_SEC_ATTRIBUTES(NULL);
+ result = SecItemDelete(attrs);
+ CFRelease(attrs);
+
+ /* We consider not found to not be an error */
+ if (result == errSecItemNotFound)
+ result = errSecSuccess;
- SecKeychainItemDelete(item);
+ return result;
}
-static void add_internet_password(void)
+static OSStatus add_internet_password(void)
{
+ CFDictionaryRef attrs;
+ OSStatus result;
+
/* Only store complete credentials */
if (!protocol || !host || !username || !password)
- return;
+ return -1;
- if (SecKeychainAddInternetPassword(
- KEYCHAIN_ARGS,
- KEYCHAIN_ITEM(password),
- NULL))
- return;
+ attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+ NULL);
+
+ result = SecItemAdd(attrs, NULL);
+ if (result == errSecDuplicateItem) {
+ CFDictionaryRef query;
+ query = CREATE_SEC_ATTRIBUTES(NULL);
+ result = SecItemUpdate(query, attrs);
+ CFRelease(query);
+ }
+
+ CFRelease(attrs);
+
+ return result;
}
static void read_credential(void)
@@ -131,36 +238,52 @@ static void read_credential(void)
if (!strcmp(buf, "protocol")) {
if (!strcmp(v, "imap"))
- protocol = kSecProtocolTypeIMAP;
+ protocol = kSecAttrProtocolIMAP;
else if (!strcmp(v, "imaps"))
- protocol = kSecProtocolTypeIMAPS;
+ protocol = kSecAttrProtocolIMAPS;
else if (!strcmp(v, "ftp"))
- protocol = kSecProtocolTypeFTP;
+ protocol = kSecAttrProtocolFTP;
else if (!strcmp(v, "ftps"))
- protocol = kSecProtocolTypeFTPS;
+ protocol = kSecAttrProtocolFTPS;
else if (!strcmp(v, "https"))
- protocol = kSecProtocolTypeHTTPS;
+ protocol = kSecAttrProtocolHTTPS;
else if (!strcmp(v, "http"))
- protocol = kSecProtocolTypeHTTP;
+ protocol = kSecAttrProtocolHTTP;
else if (!strcmp(v, "smtp"))
- protocol = kSecProtocolTypeSMTP;
- else /* we don't yet handle other protocols */
+ protocol = kSecAttrProtocolSMTP;
+ else {
+ /* we don't yet handle other protocols */
+ clear_credential();
exit(0);
+ }
}
else if (!strcmp(buf, "host")) {
char *colon = strchr(v, ':');
if (colon) {
+ UInt16 port_i;
*colon++ = '\0';
- port = atoi(colon);
+ port_i = atoi(colon);
+ port = CFNumberCreate(kCFAllocatorDefault,
+ kCFNumberShortType,
+ &port_i);
}
- host = xstrdup(v);
+ host = CFStringCreateWithCString(kCFAllocatorDefault,
+ v,
+ ENCODING);
}
else if (!strcmp(buf, "path"))
- path = xstrdup(v);
+ path = CFStringCreateWithCString(kCFAllocatorDefault,
+ v,
+ ENCODING);
else if (!strcmp(buf, "username"))
- username = xstrdup(v);
+ username = CFStringCreateWithCString(
+ kCFAllocatorDefault,
+ v,
+ ENCODING);
else if (!strcmp(buf, "password"))
- password = xstrdup(v);
+ password = CFDataCreate(kCFAllocatorDefault,
+ (UInt8 *)v,
+ strlen(v));
/*
* Ignore other lines; we don't know what they mean, but
* this future-proofs us when later versions of git do
@@ -173,6 +296,7 @@ static void read_credential(void)
int main(int argc, const char **argv)
{
+ OSStatus result = 0;
const char *usage =
"usage: git credential-osxkeychain <get|store|erase>";
@@ -182,12 +306,17 @@ int main(int argc, const char **argv)
read_credential();
if (!strcmp(argv[1], "get"))
- find_internet_password();
+ result = find_internet_password();
else if (!strcmp(argv[1], "store"))
- add_internet_password();
+ result = add_internet_password();
else if (!strcmp(argv[1], "erase"))
- delete_internet_password();
+ result = delete_internet_password();
/* otherwise, ignore unknown action */
+ if (result)
+ die("failed to %s: %d", argv[1], (int)result);
+
+ clear_credential();
+
return 0;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/4] osxkeychain: bring in line with other credential helpers
From: Bo Anderson via GitGitGadget @ 2024-02-17 23:34 UTC (permalink / raw)
To: git; +Cc: Bo Anderson
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
osxkeychain also made use of macOS APIs that had been deprecated since 2014.
Replacement API was able to be used without regressing the minimum supported
macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer
in osxkeychain, 2023-05-01).
After this set of patches, osxkeychain passes all tests in the external
credential helper test suite.
Bo Anderson (4):
osxkeychain: replace deprecated SecKeychain API
osxkeychain: erase all matching credentials
osxkeychain: erase matching passwords only
osxkeychain: store new attributes
contrib/credential/osxkeychain/Makefile | 3 +-
.../osxkeychain/git-credential-osxkeychain.c | 376 ++++++++++++++----
2 files changed, 310 insertions(+), 69 deletions(-)
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1667%2FBo98%2Fosxkeychain-update-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1667/Bo98/osxkeychain-update-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1667
--
gitgitgadget
^ permalink raw reply
* Re: Calling "gpg --sign" with loopback pinentry in some scenarios, but not always
From: Junio C Hamano @ 2024-02-17 23:31 UTC (permalink / raw)
To: Jens Schmidt; +Cc: git
In-Reply-To: <de38341b-f69f-4d3b-a4c2-4443e1ec1f6f@vodafonemail.de>
Jens Schmidt <farblos@vodafonemail.de> writes:
> It seems that "git" does not have any hooks to pass arbitrary
> additional parameters to "gpg", right? So my only reasonable
> option would be a Emacs/Magit-specific "gpg"-wrapper that adds
> command line parameter "--pinentry-mode loopback" to the wrapped
> "gpg" executable.
Correct. You can point your gpg.program configuration variable at
your wrapper, perhaps
$ cat >"$HOME/bin/mygpg-with-pinentry" <<\EOF
#!/bin/sh
exec gpg --pinentry-mode loopback "$@"
EOF
$ chmod +x "$HOME/bin/mygpg-with-pinentry"
$ git config gpg.openpgp.program "$HOME/bin/mygpg-with-pinentry"
or something like that. It is unfortunate that
$ git config gpg.openpgp.program "gpg --pinentry-mode loopback"
does not work.
^ permalink raw reply
* Calling "gpg --sign" with loopback pinentry in some scenarios, but not always
From: Jens Schmidt @ 2024-02-17 22:51 UTC (permalink / raw)
To: git
Hi.
I'd like to use gpg loopback pinentry mode when signing commits
or tags through Emacs/Magit but default, non-loopback pinentry
for all other cases.
In particular, configuring "pinentry-mode loopback" in "gpg.conf"
is not really an option for me.
It seems that "git" does not have any hooks to pass arbitrary
additional parameters to "gpg", right? So my only reasonable
option would be a Emacs/Magit-specific "gpg"-wrapper that adds
command line parameter "--pinentry-mode loopback" to the wrapped
"gpg" executable.
Any other options?
Thanks!
^ permalink raw reply
* Git commit causes data download in partial clone
From: charmocc @ 2024-02-17 20:38 UTC (permalink / raw)
To: git@vger.kernel.org
Hi everyone,
I was recently exploring git partial clone feature because I wanted to
contribute to repository which has a lot of binary files. My intent was to only
add new files without modifying any existing ones and to download as few data
as possible in the process. Here are the steps I followed:
$ git clone --no-checkout --filter=blob:none https://github.com/libretro-thumbnails/Nintendo_-_Nintendo_Entertainment_System.git nes
$ cd nes
$ echo foo > bar
$ git add bar
$ git commit bar # causes git fetch behind the scene and download of a lot of objects!
Now for reasons I don't understand the last command cause download of a lot of
objects from remote (blobs) which is what I was trying to avoid. By enabling
tracing options I can see that it runs fetch operation in the background:
git -c fetch.negotiationAlgorithm=noop fetch origin --no-tags --no-write-fetch-head --recurse-submodules=no --filter=blob:none --stdin
Unfortunately trace doesn't show what refspec is being used for the fetch
operation. The regular "git fetch" doesn't cause any additional object
downloads.
Is there something I'm doing wrong here or maybe what I'm trying to achieve
(contribute to repository without downloading all of its files) is simply
not possible?
git version 2.34.1 (Ubuntu 22.04)
Thanks for your time
charmocc
^ permalink raw reply
* Re: [PATCH v2] RelNotes: minor typo fixes in 2.44.0 draft
From: Junio C Hamano @ 2024-02-17 18:12 UTC (permalink / raw)
To: Todd Zullinger; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <20240217163753.301384-1-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> Kousik Sanagavarapu wrote:
>> Todd Zullinger <tmz@pobox.com> wrote:
>>
>>> - * When $HOME/.gitignore is missing but XDG config file available, we
>>> + * When $HOME/.gitconfig is missing but XDG config file available, we
>>
>> s/file available/file is available
>
> Good point, thanks. :)
Thanks, both. Will apply.
>
> Documentation/RelNotes/2.44.0.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/RelNotes/2.44.0.txt b/Documentation/RelNotes/2.44.0.txt
> index 7dd8d75844..7cee544eee 100644
> --- a/Documentation/RelNotes/2.44.0.txt
> +++ b/Documentation/RelNotes/2.44.0.txt
> @@ -3,7 +3,7 @@ Git v2.44 Release Notes
>
> Backward Compatibility Notes
>
> - * "git chekcout -B <branch>" used to allow switching to a branch that
> + * "git checkout -B <branch>" used to allow switching to a branch that
> is in use on another worktree, but this was by mistake. The users
> need to use "--ignore-other-worktrees" option.
>
> @@ -54,7 +54,7 @@ UI, Workflows & Features
> gitweb behaved as if the file did not exist at all, but now it
> errors out. This is a change that may break backward compatibility.
>
> - * When $HOME/.gitignore is missing but XDG config file available, we
> + * When $HOME/.gitconfig is missing but XDG config file is available, we
> should write into the latter, not former. "git gc" and "git
> maintenance" wrote into a wrong "global config" file, which have
> been corrected.
^ permalink raw reply
* [PATCH v2] RelNotes: minor typo fixes in 2.44.0 draft
From: Todd Zullinger @ 2024-02-17 16:37 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: Junio C Hamano, git
In-Reply-To: <ZdAX1Sfiq6gJCoEk@five231003>
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
Kousik Sanagavarapu wrote:
> Todd Zullinger <tmz@pobox.com> wrote:
>
>> - * When $HOME/.gitignore is missing but XDG config file available, we
>> + * When $HOME/.gitconfig is missing but XDG config file available, we
>
> s/file available/file is available
Good point, thanks. :)
Documentation/RelNotes/2.44.0.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/RelNotes/2.44.0.txt b/Documentation/RelNotes/2.44.0.txt
index 7dd8d75844..7cee544eee 100644
--- a/Documentation/RelNotes/2.44.0.txt
+++ b/Documentation/RelNotes/2.44.0.txt
@@ -3,7 +3,7 @@ Git v2.44 Release Notes
Backward Compatibility Notes
- * "git chekcout -B <branch>" used to allow switching to a branch that
+ * "git checkout -B <branch>" used to allow switching to a branch that
is in use on another worktree, but this was by mistake. The users
need to use "--ignore-other-worktrees" option.
@@ -54,7 +54,7 @@ UI, Workflows & Features
gitweb behaved as if the file did not exist at all, but now it
errors out. This is a change that may break backward compatibility.
- * When $HOME/.gitignore is missing but XDG config file available, we
+ * When $HOME/.gitconfig is missing but XDG config file is available, we
should write into the latter, not former. "git gc" and "git
maintenance" wrote into a wrong "global config" file, which have
been corrected.
--
2.44.0.rc1
^ permalink raw reply related
* [PATCH v4] mergetools: vimdiff: use correct tool's name when reading mergetool config
From: Kipras Melnikovas @ 2024-02-17 16:27 UTC (permalink / raw)
To: git; +Cc: gitster, greenfoo, Kipras Melnikovas
In-Reply-To: <20240217074343.12608-1-kipras@kipras.org>
The /mergetools/vimdiff script, which handles both vimdiff, nvimdiff
and gvimdiff mergetools (the latter 2 simply source the vimdiff script), has a
function merge_cmd() which read the layout variable from git config, and it
would always read the value of mergetool.**vimdiff**.layout, instead of the
mergetool being currently used (vimdiff or nvimdiff or gvimdiff).
It looks like in 7b5cf8be18 (vimdiff: add tool documentation, 2022-03-30),
we explained the current behavior in Documentation/config/mergetool.txt:
```
mergetool.vimdiff.layout::
The vimdiff backend uses this variable to control how its split
windows look like. Applies even if you are using Neovim (`nvim`) or
gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
```
which makes sense why it's explained this way - the vimdiff backend is used by
gvim and nvim. But the mergetool's configuration should be separate for each tool,
and indeed that's confirmed in same commit at Documentation/mergetools/vimdiff.txt:
```
Variants
Instead of `--tool=vimdiff`, you can also use one of these other variants:
* `--tool=gvimdiff`, to open gVim instead of Vim.
* `--tool=nvimdiff`, to open Neovim instead of Vim.
When using these variants, in order to specify a custom layout you will have to
set configuration variables `mergetool.gvimdiff.layout` and
`mergetool.nvimdiff.layout` instead of `mergetool.vimdiff.layout`
```
So it looks like we just forgot to update the 1 part of the vimdiff script
that read the config variable. Cheers.
Though, for backward compatibility, I've kept the mergetool.vimdiff
fallback, so that people who unknowingly relied on it, won't have their
setup broken now.
Signed-off-by: Kipras Melnikovas <kipras@kipras.org>
---
Okay I've finalised the documentation, should be the last patch.
Also I realise I've forgotten to cc the mailing list on my replies to
Junio and Fernando - sorry! First time..
Range-diff against v3:
1: 0018c7e18c = 1: 0018c7e18c mergetools: vimdiff: use correct tool's name when reading mergetool config
Documentation/config/mergetool.txt | 21 ++++++++++++++-------
Documentation/mergetools/vimdiff.txt | 3 ++-
mergetools/vimdiff | 12 ++++++++++--
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 294f61efd1..00bf665aa0 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -45,14 +45,21 @@ mergetool.meld.useAutoMerge::
value of `false` avoids using `--auto-merge` altogether, and is the
default value.
-mergetool.vimdiff.layout::
- The vimdiff backend uses this variable to control how its split
- windows appear. Applies even if you are using Neovim (`nvim`) or
- gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
-ifndef::git-mergetool[]
- in linkgit:git-mergetool[1].
+mergetool.<vimdiff variant>.layout::
+ Configure the split window layout for vimdiff's `<variant>`, which is any of `vimdiff`,
+ `nvimdiff`, `gvimdiff`.
+ Upon launching `git mergetool` with `--tool=<variant>` (or without `--tool`
+ if `merge.tool` is configured as `<variant>`), Git will consult
+ `mergetool.<variant>.layout` to determine the tool's layout. If the
+ variant-specific configuration is not available, `vimdiff`'s is used as
+ fallback. If that too is not available, a default layout with 4 windows
+ will be used. To configure the layout, see the `BACKEND SPECIFIC HINTS`
+ifdef::git-mergetool[]
+ section.
+endif::[]
+ifndef::git-mergetool[]
+ section in linkgit:git-mergetool[1].
endif::[]
- for details.
mergetool.hideResolved::
During a merge, Git will automatically resolve as many conflicts as
diff --git a/Documentation/mergetools/vimdiff.txt b/Documentation/mergetools/vimdiff.txt
index d1a4c468e6..befa86d692 100644
--- a/Documentation/mergetools/vimdiff.txt
+++ b/Documentation/mergetools/vimdiff.txt
@@ -177,7 +177,8 @@ Instead of `--tool=vimdiff`, you can also use one of these other variants:
When using these variants, in order to specify a custom layout you will have to
set configuration variables `mergetool.gvimdiff.layout` and
-`mergetool.nvimdiff.layout` instead of `mergetool.vimdiff.layout`
+`mergetool.nvimdiff.layout` instead of `mergetool.vimdiff.layout` (though the
+latter will be used as fallback if the variant-specific one is not set).
In addition, for backwards compatibility with previous Git versions, you can
also append `1`, `2` or `3` to either `vimdiff` or any of the variants (ex:
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 06937acbf5..97e376329b 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -371,9 +371,17 @@ diff_cmd_help () {
merge_cmd () {
- layout=$(git config mergetool.vimdiff.layout)
+ TOOL=$1
- case "$1" in
+ layout=$(git config "mergetool.$TOOL.layout")
+
+ # backward compatibility:
+ if test -z "$layout"
+ then
+ layout=$(git config mergetool.vimdiff.layout)
+ fi
+
+ case "$TOOL" in
*vimdiff)
if test -z "$layout"
then
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
2.43.1
^ permalink raw reply related
* Re: [PATCH] branch: rework the descriptions of rename and copy operations
From: Rubén Justo @ 2024-02-17 14:58 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <ea15a49aed7b5a74cd9b1bf8a5351df9@manjaro.org>
On 16-feb-2024 04:32:10, Dragan Simic wrote:
> On 2024-02-16 00:34, Rubén Justo wrote:
> > On 15-feb-2024 14:13:31, Junio C Hamano wrote:
> > > Rubén Justo <rjusto@gmail.com> writes:
> > > > On 15-feb-2024 19:42:32, Dragan Simic wrote:
> > > > Let me chime in just to say that maybe another terms could be
> > > > considered
> > > > here; like: "<branchname>" and "<newbranchname>" (maybe too
> > > > long...) or
> > > > so.
> > > >
> > > > I have no problem with the current terms, but "<branchname>" can be a
> > > > sensible choice here as it is already being used for other commands
> > > > where, and this may help overall, the consideration: "if ommited, the
> > > > current branch is considered" also applies.
> > >
> > > Actually, we should go in the opposite direction. When the use of
> > > names are localized in a narrower context, they can be shortened
> > > without losing clarity.
> >
> > I did not mean to have longer terms, sorry for that.
> >
> > I was thinking more in the synopsis:
> >
> > 'git branch' (--set-upstream-to=<upstream> | -u <upstream>)
> > [<branchname>]
> > 'git branch' --unset-upstream [<branchname>]
> > 'git branch' (-m | -M) [<branchname>] <new>
> > 'git branch' (-c | -C) [<branchname>] <new>
> > 'git branch' (-d | -D) [-r] <branchname>...
> > 'git branch' --edit-description [<branchname>]
> >
> > To have more uniformity in the terms, which can be beneficial to the
> > user.
>
> 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.
I find it interesting to have common terms like "<commit>" "<path>",
"<envvar>"...
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.
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/
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.
As I write, I realise that all this probably belongs to CodingGuideLines
or similar. Or maybe it is too academic to be practical. I Dunno.
> '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>:
> 'git branch' (-c | -C) [<old>] <new>
> 'git branch' (-d | -D) [-r] <name>...
At least, not better than with <oldbranch> and <branchname>.
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>
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.
> Though, it's something to be left for future patches, which will move
> more argument descriptions to the respective command descriptions.
>
> > 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 :)
> because the description of the command for
> editing branch descriptions is detached from the description of its
> arguments. The plan is to move all of them together.
>
> > Of course, there is no need (perhaps counterproductive) to say "branch"
> > if the context makes it clear it is referring to a branch.
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Maarten Bosmans @ 2024-02-17 12:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Teng Long
In-Reply-To: <20240215150430.GA3453@coredump.intra.peff.net>
Op do 15 feb 2024 om 16:04 schreef Jeff King <peff@peff.net>:
>
> On Thu, Feb 15, 2024 at 08:46:02AM +0100, Maarten Bosmans wrote:
>
> > > How about:
> > >
> > > cat some_commit_ids |
> > > git show --stdin -s -z --format='%H%n%N'
> > >
> > Wouldn't that fail horribly with non-text blobs?
>
> Yes, if you have NULs embedded in your notes then it won't work. Any
> batch output format would require byte counts, then. If we wanted to add
> a feature to support that, I would suggest one of:
>
> - teach the pretty-print formatter a new placeholder to output the
> number of bytes in an element. Then you could do something like
> "%H %(size:%N)%n%N", but it would be generally useful for other
> cases, too.
>
> - teach the pretty-print formatter a variant of %N that outputs only
> the oid of the note, note the note content itself. And then you
> could do something like:
>
> git log --format='%(note:oid) %H' |
> git cat-file --batch='%(objectname) %(objectsize) %(rest)'
>
> to get the usual cat-file output of each note blob, but associated
> with the commit it's attached to (the "%(rest)" placeholder for
> cat-file just relays any text found after the object name of each
> line). You might need to do some scripting between the two to handle
> commits with no note.
>
> Of the two, I'd guess that the second one is a lot less work to
> implement (on the Git side; on the reading side it's a little more
> involved, but still should be a constant number of processes).
The second one is attractive for another reason than implementation
simplicity. While the first one offers more flexibility, the second
reuses the existing cat-file batch format, so the interface between
git and scripts is familiar and consistent.
> One variant of the second one is to use "git notes list". For example,
> you can get all notes via cat-file like this right now:
>
> git notes list |
> git cat-file --batch='%(objectname) %(objectsize) %(rest)'
So the cat-file batch output is suitable for blobs containing newline
or NUL characters. But I struggle a bit with what would be an easy way
of using this format in a shell script. Something with multiple read
and read -N commands reading from the output, I guess.
The git codebase has `extract_batch_output()` in t1006. This uses a
separate perl invocation to parse the cat-file output, which confirms
my suspicion there isn't a straight-forward way to do this in e.g.
just a bash script.
That was why my first steps were to accept that a launching a separate
process per note in a bash loop is a pretty clear and well understood
idiom in shell scripts and try to make the git part of that a bit more
efficient.
> You can get individual notes by asking for "git notes list <commit>",
> but it will only take one at a time. So another easy patch would be
> something like (indentation left funny to make the diff more readable):
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index e65cae0bcf..5fdad5fb8f 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -446,22 +446,22 @@ static int list(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, options,
> git_notes_list_usage, 0);
>
> - if (1 < argc) {
> - error(_("too many arguments"));
> - usage_with_options(git_notes_list_usage, options);
> - }
> -
> t = init_notes_check("list", 0);
> if (argc) {
> - if (repo_get_oid(the_repository, argv[0], &object))
> - die(_("failed to resolve '%s' as a valid ref."), argv[0]);
> + retval = 0;
> + while (*++argv) {
> + if (repo_get_oid(the_repository, *argv, &object))
> + die(_("failed to resolve '%s' as a valid ref."), *argv);
> note = get_note(t, &object);
> if (note) {
> - puts(oid_to_hex(note));
> - retval = 0;
> + if (argc > 1)
> + printf("%s %s\n", oid_to_hex(note), oid_to_hex(&object));
> + else
> + puts(oid_to_hex(note));
> } else
> - retval = error(_("no note found for object %s."),
> + retval |= error(_("no note found for object %s."),
> oid_to_hex(&object));
> + }
> } else
> retval = for_each_note(t, 0, list_each_note, NULL);
>
>
> That would allow:
>
> git rev-list ... |
> xargs git notes list |
> git cat-file --batch='%(objectname) %(objectsize) %(rest)'
>
> We could even add a "--stdin" mode to avoid the use of xargs.
Yes, a --stdin mode for `git notes list` would be a useful building
block for scripting indeed.
I'll probably give it a try when this patch series succeeds.
Maarten
^ permalink raw reply
* [PATCH v3] mergetools: vimdiff: use correct tool's name when reading mergetool config
From: Kipras Melnikovas @ 2024-02-17 7:43 UTC (permalink / raw)
To: git; +Cc: gitster, greenfoo, Kipras Melnikovas
In-Reply-To: <20240215142002.36870-1-kipras@kipras.org>
The /mergetools/vimdiff script, which handles both vimdiff, nvimdiff
and gvimdiff mergetools (the latter 2 simply source the vimdiff script), has a
function merge_cmd() which read the layout variable from git config, and it
would always read the value of mergetool.**vimdiff**.layout, instead of the
mergetool being currently used (vimdiff or nvimdiff or gvimdiff).
It looks like in 7b5cf8be18 (vimdiff: add tool documentation, 2022-03-30),
we explained the current behavior in Documentation/config/mergetool.txt:
```
mergetool.vimdiff.layout::
The vimdiff backend uses this variable to control how its split
windows look like. Applies even if you are using Neovim (`nvim`) or
gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
```
which makes sense why it's explained this way - the vimdiff backend is used by
gvim and nvim. But the mergetool's configuration should be separate for each tool,
and indeed that's confirmed in same commit at Documentation/mergetools/vimdiff.txt:
```
Variants
Instead of `--tool=vimdiff`, you can also use one of these other variants:
* `--tool=gvimdiff`, to open gVim instead of Vim.
* `--tool=nvimdiff`, to open Neovim instead of Vim.
When using these variants, in order to specify a custom layout you will have to
set configuration variables `mergetool.gvimdiff.layout` and
`mergetool.nvimdiff.layout` instead of `mergetool.vimdiff.layout`
```
So it looks like we just forgot to update the 1 part of the vimdiff script
that read the config variable. Cheers.
Though, for backward compatibility, I've kept the mergetool.vimdiff
fallback, so that people who unknowingly relied on it, won't have their
setup broken now.
Signed-off-by: Kipras Melnikovas <kipras@kipras.org>
---
Here's some variants I considered for mergetool.<vimdiff variant>.layout
in Documentation/config/mergetool.txt, but discarded for a shorter
version. Feel free to pick & edit the final.
a)
The `<vimdiff variant>` is any of `vimdiff`, `nvimdiff`, `gvimdiff`. When
you run `git mergetool` with `--tool=<vimdiff variant>`, Git will consult
`mergetool.<vimdiff variant>.layout` to determine the tool's layout. If it's
not specified, `vimdiff`'s is used as fallback. If that too is not available,
a default layout with 4 windows is used. See BACKEND SPECIFIC HINTS section
ifndef::git-mergetool[]
in linkgit:git-mergetool[1]
endif::[]
for details.
b)
Configure a custom layout for your mergetool. The `<variant>` is any
of `vimdiff`, `nvimdiff`, `gvimdiff`.
Upon launching `git mergetool` with `--tool=<variant>` (or without
`--tool` if `merge.tool` is configured as `<variant>`), Git
will consult `mergetool.<vimdiff variant>.layout` to determine the tool's
layout. If the variant-specific config is not available, `vimdiff`'s is
used as fallback. If that too is not available, a default layout with 4
windows will be used. See BACKEND SPECIFIC HINTS section
ifndef::git-mergetool[]
in linkgit:git-mergetool[1]
endif::[]
for details.
The ifdef + ifndef is used to avoid an extra space before the final "."
Range-diff against v2:
1: 070280d95d ! 1: 60be87c3d5 mergetools: vimdiff: use correct tool's name when reading mergetool config
@@ Documentation/config/mergetool.txt: mergetool.meld.useAutoMerge::
- The vimdiff backend uses this variable to control how its split
- windows appear. Applies even if you are using Neovim (`nvim`) or
- gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
-+mergetool.{g,n,}vimdiff.layout::
-+ The vimdiff backend uses this variable to control how its split windows
-+ appear. Use `mergetool.vimdiff` for regular Vim, `mergetool.nvimdiff` for
-+ Neovim and `mergetool.gvimdiff` for gVim to configure the merge tool. See
-+ BACKEND SPECIFIC HINTS section
- ifndef::git-mergetool[]
- in linkgit:git-mergetool[1].
+-ifndef::git-mergetool[]
+- in linkgit:git-mergetool[1].
++mergetool.<vimdiff variant>.layout::
++ Git's vimdiff backend uses this variable to control how the split windows of
++ `<vimdiff variant>` appear. Here `<vimdiff variant>` is any of `vimdiff`,
++ `nvimdiff`, `gvimdiff`. To configure the layout and use the tool, see the
++ `BACKEND SPECIFIC HINTS`
++ifdef::git-mergetool[]
++ section.
++endif::[]
++ifndef::git-mergetool[]
++ section in linkgit:git-mergetool[1].
endif::[]
+- for details.
+
+ mergetool.hideResolved::
+ During a merge, Git will automatically resolve as many conflicts as
## mergetools/vimdiff ##
@@ mergetools/vimdiff: diff_cmd_help () {
@@ mergetools/vimdiff: diff_cmd_help () {
+ TOOL=$1
- case "$1" in
-+ layout=$(git config mergetool.$TOOL.layout)
++ layout=$(git config "mergetool.$TOOL.layout")
+
-+ # backwards-compatibility:
++ # backward compatibility:
+ if test -z "$layout"
+ then
+ layout=$(git config mergetool.vimdiff.layout)
Documentation/config/mergetool.txt | 17 ++++++++++-------
mergetools/vimdiff | 12 ++++++++++--
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 294f61efd1..f79c798b74 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -45,14 +45,17 @@ mergetool.meld.useAutoMerge::
value of `false` avoids using `--auto-merge` altogether, and is the
default value.
-mergetool.vimdiff.layout::
- The vimdiff backend uses this variable to control how its split
- windows appear. Applies even if you are using Neovim (`nvim`) or
- gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
-ifndef::git-mergetool[]
- in linkgit:git-mergetool[1].
+mergetool.<vimdiff variant>.layout::
+ Git's vimdiff backend uses this variable to control how the split windows of
+ `<vimdiff variant>` appear. Here `<vimdiff variant>` is any of `vimdiff`,
+ `nvimdiff`, `gvimdiff`. To configure the layout and use the tool, see the
+ `BACKEND SPECIFIC HINTS`
+ifdef::git-mergetool[]
+ section.
+endif::[]
+ifndef::git-mergetool[]
+ section in linkgit:git-mergetool[1].
endif::[]
- for details.
mergetool.hideResolved::
During a merge, Git will automatically resolve as many conflicts as
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 06937acbf5..97e376329b 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -371,9 +371,17 @@ diff_cmd_help () {
merge_cmd () {
- layout=$(git config mergetool.vimdiff.layout)
+ TOOL=$1
- case "$1" in
+ layout=$(git config "mergetool.$TOOL.layout")
+
+ # backward compatibility:
+ if test -z "$layout"
+ then
+ layout=$(git config mergetool.vimdiff.layout)
+ fi
+
+ case "$TOOL" in
*vimdiff)
if test -z "$layout"
then
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
2.43.1
^ permalink raw reply related
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-17 6:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqqwmr3fxc1.fsf@gitster.g>
On Fri, Feb 16, 2024 at 09:56:14PM -0800, Junio C Hamano wrote:
> > Hmm, good question. I can't think offhand of a way that the user could
> > convince "git show <oid>" to do anything different than just dumping the
> > literal contents. It is not even handed a path that could trigger
> > .gitattributes or similar.
>
> show_blob_object() directly calls stream_blob_to_fd() without any
> filter, as the hardcoded invocation of "git show <oid>" in the note
> codepath does not allow --textconv at all, so we probably are safe
> to assume that the contents will appear as-is, not even going
> through EOL conversion (which is a bit puzzling, to be honest,
> though). Lack of path I am not worried about, as you can easily
> declare with '*' wildcard that everything in the notes tree is of
> the same type. But if the stream_blob_to_fd() interface does not
> have anywhere smudge filters or textconv filters can hook into
> without some command line option, we do not have to worry about it.
I think we are mostly on the same page, but just to be pedantic: I do
not think even adding a "*" attribute in the notes tree could ever do
anything. The "git show" invocation is not "git show notes_ref:<path>".
We resolve the note blob to an oid, and then pass that oid's hex to "git
show".
So the "git show" command itself has no context at all, and does not
even know this blob came from a tree in the first place.
-Peff
^ permalink raw reply
* Re: [BUG] git commit --trailer --verbose puts trailer below scissors line
From: Jeff King @ 2024-02-17 6:04 UTC (permalink / raw)
To: Philippe Blain; +Cc: Linus Arver, Git mailing list
In-Reply-To: <8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com>
On Fri, Feb 16, 2024 at 04:04:18PM -0500, Philippe Blain wrote:
> Hello,
>
> I've just found a bug in the current master: running
>
> git commit --trailer="key: value" --verbose
>
> puts the trailer below the diff, and thus below the scissors
> line (and so it is discarded).
>
> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not
> a new regression in the 2.44 cycle, but I thought I'd write now in case
> someone spots the culprit commit faster than me.
>
> I'll start a bisection now.
Looks like it bisects to 97e9d0b78a (trailer: find the end of the log
message, 2023-10-20). Here's a test that demonstrates it (signed-off-by:
me in case anyone wants to incorporate it into a fix):
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b5bf7de7cd..d8e216613f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
test_cmp expected actual
'
+test_expect_success 'commit --trailer with --verbose' '
+ cat >msg <<-\EOF &&
+ subject
+
+ body
+ EOF
+ GIT_EDITOR=: git commit --edit -F msg --allow-empty \
+ --trailer="my-trailer: value" --verbose &&
+ {
+ cat msg &&
+ echo &&
+ echo "my-trailer: value"
+ } >expected &&
+ git cat-file commit HEAD >commit.msg &&
+ sed -e "1,/^\$/d" commit.msg >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
-Peff
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox