2024. nov. 15. 23:18:10 Bence Ferdinandy <bence@xxxxxxxxxxxxxx>:
On Fri Nov 15, 2024 at 06:50, Junio C Hamano <gitster@xxxxxxxxx> wrote:
Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes:
int refs_update_symref(struct ref_store *refs, const char *ref,
const char *target, const char *logmsg)
+{
+ return refs_update_symref_extended(refs, ref, target, logmsg,
NULL);
+}
OK. As the enhanced and renamed function is also external, we do
not have to worry about reordering the old one to come after the new
one.
I guess this also decides that the name "_extended" is fine :)
+int refs_update_symref_extended(struct ref_store *refs, const char
*ref,
+ const char *target, const char *logmsg,
+ struct strbuf *referent)
{
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -2122,13 +2129,20 @@ int refs_update_symref(struct ref_store
*refs, const char *ref,
transaction = ref_store_transaction_begin(refs, &err);
if (!transaction ||
- ref_transaction_update(transaction, ref, NULL, NULL,
+ ref_transaction_update(transaction, ref, NULL, NULL,
An unwanted patch noise?
target, NULL, REF_NO_DEREF,
logmsg, &err) ||
- ref_transaction_commit(transaction, &err)) {
+ ref_transaction_prepare(transaction, &err)) {
Likewise, but the noise distracts from the real change made on this
line, which is even worse.
Mea culpa, I'll get this cleaned up.
The real change here is to only call _prepare(), which also asks the
transaction hook if we are OK to proceed. If we fail, we stop here
ret = error("%s", err.buf);
+ goto cleanup;
This is also a real change. We could instead make the additional
code below into the else clause (see below).
}
+ if (referent)
+ refs_read_symbolic_ref(refs, ref, referent);
And if we were asked to give the value of the symbolic ref, we make
this call. What should this code do when reading fails (I know it
ignores, as written, but I am asking what it _should_ do)?
I think this should do _nothing_ if it fails (although should it stay
this way,
I guess it should be marked with a comment that this is on purpose). My
reasoning is that running `git fetch` will be running this part of the
code,
which means that should reading the symbolic ref fail for any reason, a
`fetch`
that previously ran without error would now fail. We now pass up an
empty
string as the previous which does mask that there was an error here.
What
I think we could maybe do is pass up a special string that means there
was an
error? Something that for sure cannot be a valid value for an existing
reference? I'm not sure how much sense that makes.
Sorry, it's late. The above is slightly bollocks since fetch ignores any
set_head errors later :)
But the idea stands that if we can set the head, let's do it.
The previous head is not important enough to die on.
As mentioned on the other patch, we could try to read a non-symref
instead also, if symref fails.
+ if (ref_transaction_commit(transaction, &err))
+ ret = error("%s", err.buf);
And then we commit, or we fail to commit.
+cleanup:
We could write the whole thing as a single "do these and leave as
soon as we see any failure" ||-cascade,
if (!(transaction = ref_store_transaction_begin(refs, &err)) ||
ref_transaction_update(transaction, ref, NULL, NULL,
target, NULL, REF_NO_DEREF,
logmsg, &err) ||
ref_transaction_prepare(transaction, &err)) ||
(referent
? refs_read_symbolic_ref(refs, ref, referent)
: 0) ||
ref_transaction_commit(transaction, &err)) {
if (!err.len)
... stuff default error message to err ...;
ret = error("%s", err.buf);
}
which may not necessarily easier to follow (and in fact it is rather
ugly), but at least, the resulting code does not have to special
case the "optionally peek into the symref" step.
As I said above, I don't think we actually want to fail the update even
if the
symbolic ref reading fails, so I think the special casing should stay.
I'll
wait here to see more clearly on what to do here.
