From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 10 Apr 2026 15:39:14 GMT
Subject: Re: [PATCH v8 1/4] stash: add --ours-label, --theirs-label, --base-label for apply
Message-ID: <0d1c7bf2-6404-4779-a0d6-6db592510a04@gmail.com>
In-Reply-To: <8fcf3778205d4742a56ed2e4c3b97defa21a1538.1775762235.git.gitgitgadget@gmail.com>

Hi Harald

On 09/04/2026 20:17, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> 
> Allow callers of "git stash apply" to pass custom labels for conflict
> markers instead of the default "Updated upstream" and "Stashed changes".
> Document the new options and add a test.

Sounds sensible and the documentation looks good.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 0d27b2fb1f..54bcb6ac73 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -44,7 +44,7 @@
>   #define BUILTIN_STASH_POP_USAGE \
>   	N_("git stash pop [--index] [-q | --quiet] [<stash>]")
>   #define BUILTIN_STASH_APPLY_USAGE \
> -	N_("git stash apply [--index] [-q | --quiet] [<stash>]")
> +	N_("git stash apply [--index] [-q | --quiet] [--ours-label=<label>] [--theirs-label=<label>] [--base-label=<label>] [<stash>]")
>   #define BUILTIN_STASH_BRANCH_USAGE \
>   	N_("git stash branch <branchname> [<stash>]")
>   #define BUILTIN_STASH_STORE_USAGE \

This patch seems to be missing the implementation of these new options. 
Before submitting a patch series I find it is very helpful to run

     git rebase --keep-base -x make -x 'cd t && prove -j6 <tests that I 
think might fail>'

to catch any mistakes.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..d4e4e4d7b6 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1666,6 +1666,43 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
>   	)
>   '
>   
> +test_expect_success 'apply with custom conflict labels' '
> +	git init conflict_labels &&

Why do we need to create a new repository just to stash some changes?

> +	(
> +		cd conflict_labels &&
> +		echo base >file &&
> +		git add file &&
> +		git commit -m base &&

We have a helper test_commit() for creating commits (it is documented in 
t/test-lib-functions.sh)

> +		echo stashed >file &&
> +		git stash push -m "stashed" &&
> +		echo upstream >file &&
> +		git add file &&
> +		git commit -m upstream &&
> +		test_must_fail git -c merge.conflictStyle=diff3 stash apply --ours-label=UP --theirs-label=STASH &&
> +		test_grep "^<<<<<<< UP" file &&
> +		test_grep "^||||||| Stash base" file &&
> +		test_grep "^>>>>>>> STASH" file

Hurray for the use of test_grep here!

> +	)
> +'
> +
> +test_expect_success 'apply with empty conflict labels' '

Why do we want to support empty labels rather than making them an error?

Thanks

Phillip

> +	git init empty_labels &&
> +	(
> +		cd empty_labels &&
> +		echo base >file &&
> +		git add file &&
> +		git commit -m base &&
> +		echo stashed >file &&
> +		git stash push -m "stashed" &&
> +		echo upstream >file &&
> +		git add file &&
> +		git commit -m upstream &&
> +		test_must_fail git stash apply --ours-label= --theirs-label= &&
> +		test_grep "^<<<<<<<$" file &&
> +		test_grep "^>>>>>>>$" file
> +	)
> +'
> +
>   test_expect_success 'stash create reports a locked index' '
>   	test_when_finished "rm -rf repo" &&
>   	git init repo &&
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 29dad98c49..659ad4ec97 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -199,9 +199,9 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>   			      int size, int i, int style,
>   			      xdmerge_t *m, char *dest, int marker_size)
>   {
> -	int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> -	int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> -	int marker3_size = (name3 ? strlen(name3) + 1 : 0);
> +	int marker1_size = (name1 && *name1 ? strlen(name1) + 1 : 0);
> +	int marker2_size = (name2 && *name2 ? strlen(name2) + 1 : 0);
> +	int marker3_size = (name3 && *name3 ? strlen(name3) + 1 : 0);
>   	int needs_cr = is_cr_needed(xe1, xe2, m);
>   
>   	if (marker_size <= 0)