* Re: pull fails after commit dry-run
2022-02-22 6:11 Maximilian Reichel
@ 2022-02-22 23:15 ` Rafael Silva
0 siblings, 0 replies; 3+ messages in thread
From: Rafael Silva @ 2022-02-22 23:15 UTC (permalink / raw)
To: Maximilian Reichel; +Cc: git
Hi,
Maximilian Reichel <reichemn@icloud.com> writes:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> Running the following two scripts.
>
>
> Script one:
> '#!/bin/bash
>
> mkdir parent
> git -C parent init
> git -C parent -c user.name="P" -c user.email="m@example.com" commit -m one --allow-empty
>
> mkdir cloneDir
> cd cloneDir
> git init
>
> git pull -v --rebase "../parent"
> echo git pull exit code: $?'
>
>
>
>
> script two:
> '#!/bin/bash
>
> mkdir parent
> git -C parent init
> git -C parent -c user.name="P" -c user.email="m@example.com" commit -m one --allow-empty
>
> mkdir cloneDir
> cd cloneDir
> git init
> git commit -m "foo" --dry-run
> git pull -v --rebase "../parent"
> echo git pull exit code: $?'
>
Thanks for providing the script to quickly assess the problem.
>
> What did you expect to happen? (Expected behavior)
> Since they only differ in the 'git commit -m "foo" --dry-run' invocation, I would expect the same outcome for both scripts.
> Expected output of the last two lines:
> 'From ../parent
> * branch HEAD -> FETCH_HEAD
> git pull exit code: 0'
>
> What happened instead? (Actual behavior)
> The second script is not able to pull from ../parent.
> Output of the last lines of the second script:
> 'fatal: Updating an unborn branch with changes added to the index.
> git pull exit code: 128’
>
First of all, as I'm not an experienced contributor, the following
should be taken with a grain of salt. Nevertheless, I thought about
given a shot here.
It seems this boils down to the fact that on the second script, `git
commit --dry-run` will create the `.git/index` (the Git Index) in order
to run the commit process as part of its natural workflow (i.e: `git
add`, `git commit`). Even though the index has no entries in it.
Later, when `git pull --rebase` is called, it executes the following
condition that evaluates to true (as we can see from the report ;) ):
# builtin/pull.c:cmd_pull
if (is_null_oid(&orig_head) && !is_cache_unborn())
die(_("Updating an unborn branch with changes added to the index."));
On the condition, according to my understand of the code, we die when
.git/HEAD does not point to a commit and when the index is there (or
born, to use the code terminology). The first will be true, on this
particular case, because it's a fresh repository.
IOW, we assume that is not safe to rebase on a "yet to be born branch"
with a non-empty index, as the index is not based on anything that comes
from the upstream repository. This indicates that there local changes
that will be destroy if the rebase is performed.
This behavior dates back to 19a7fcbf16 (allow pull --rebase on branch
yet to be born, 2009-08-11). Back then, `git pull` was written in shell
script with the following code:
# On an unborn branch
if test -f "$GIT_DIR/index"
then
die "updating an unborn branch with changes added to the index"
fi
On the current code (e6ebfd0e8c (The sixth batch, 2022-02-18)), we
identify if the index is not born, by checking if the index has no
entries and if the timestamp is set:
#define is_cache_unborn() is_index_unborn(&the_index)
int is_index_unborn(struct index_state *istate)
{
return (!istate->cache_nr && !istate->timestamp.sec);
}
Now, this is where it gets a bit interesting. On this case, the index
created by the `commit --dry-run` is empty in a sense: It doesn't have
any entries. Which makes me think that we "should" be able to `pull
--rebase`, for such case. For instance, by untighting the condition to
only check the number of entries. However, I'm probably missing some
intrinsic information that will break the code or worse deleting local
changes.
Just in order to illustrate better how such change can look like:
--- 8< ---
diff --git a/builtin/pull.c b/builtin/pull.c
index 3768552e68..c45a880e56 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -980,6 +980,20 @@ static void show_advice_pull_non_ff(void)
"invocation.\n"));
}
+/*
+ * If, for some reason, this turns out to be a good idea,
+ * it shuold be move to its proper location.
+ */
+#define is_cache_empty() is_index_empty(&the_index)
+
+static int is_index_empty(struct index_state *istate)
+{
+ return !istate->cache_nr;
+}
+
+int is_index_unborn(struct index_state *);
+
+
int cmd_pull(int argc, const char **argv, const char *prefix)
{
const char *repo, **refspecs;
@@ -1043,7 +1057,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (opt_autostash == -1)
opt_autostash = config_autostash;
- if (is_null_oid(&orig_head) && !is_cache_unborn())
+ if (is_null_oid(&orig_head) && !is_cache_empty())
die(_("Updating an unborn branch with changes added to the index."));
if (!opt_autostash)
--- >8 ---
Whether this is a good idea or not, I'm not able to tell right now. If
this turns out to be a good idea, I can prepare a follow up patch with
proper testing.
>
> Anything else you want to add:
> I tested this on git 2.35.1, 2.34.1 and 2.21.0 and they are all affected.
>
>
> [System Info]
> git version:
> git version 2.35.1
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> uname: Linux 5.10.25-linuxkit #1 SMP Tue Mar 23 09:27:39 UTC 2021 x86_64
> compiler info: gnuc: 10.2
> libc info: glibc: 2.31
> $SHELL (typically, interactive shell): <unset>
>
>
> [Enabled Hooks]
> not run from a git repository - no hooks to show
--
Thanks
Raffs
^ permalink raw reply related [flat|nested] 3+ messages in thread