Re: [Outreachy PATCH v2 2/2] apply: reject new and deleted files that have both names

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 14/11/26 06:50, Tanga Joris wrote:
> Git writes "new file mode" or "deleted file mode" before the "---"
> and "+++" lines. The "new file mode" line sets is_new. Then
> gitdiff_oldname() leaves old_name empty for "--- /dev/null".
> 
> When the "---" line comes first, is_new is still 0. So
> gitdiff_verify_name() takes the name on that line as old_name. For
> "--- /dev/null" this gives "dev/null", because one leading directory
> is stripped. The "new file mode" line then sets is_new. The result is
> a new file that also has an old name. "git apply --check" dies on it,
> on the same assertion in check_preimage(). In the same way,
> "+++ /dev/null" before "deleted file mode" gives a deleted file with
> a new name. "git apply -R" dies on that one.
> 
> d70e9c5c8c (apply: check git diffs for mutually exclusive header
> lines, 2017-06-27) made apply report other conflicting header lines,
> instead of leaving them to an assertion. Do the same here. After the
> header is read, reject a new file that has an old name. Reject a
> deleted file that has a new name too.
> 
> Git always writes the mode line first, so patches made by Git are not
> affected. A patch with
> "+++ /dev/null" before "deleted file mode" used to apply without -R.
> It is now rejected too.
> 
> Signed-off-by: Tanga Joris <tangajoris@gmail.com>
> ---
>   apply.c                    | 20 ++++++++++++++++++++
>   t/t4133-apply-filenames.sh | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 51 insertions(+)
> 
> diff --git a/apply.c b/apply.c
> index c5d4fe7887..0fa6a15a65 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1466,6 +1466,26 @@ int parse_git_diff_header(struct strbuf *root,
>   			      *linenr);
>   		return -128;
>   	}
> +	if (patch->is_new > 0 && patch->old_name) {
> +		if (patch_input_file)
> +			error(_("git diff header for new file %s names an old file %s at %s:%d"),
> +			      patch->new_name, patch->old_name,


Nit: for "--- /dev/null" this prints "names an old file dev/null", but 
the users never write "dev/null", that's what's left after one leading 
component is stripped. And "--- a/f" gives "names an old file f". The 
only way to reach here is a "---" before "new file mode", so can't we 
name that instead of printing the old name? Same for the deleted case 
with "+++" before "deleted file mode".

Thanks,
Siddharth


> +			      patch_input_file, *linenr);
> +		else
> +			error(_("git diff header for new file %s names an old file %s (line %d)"),
> +			      patch->new_name, patch->old_name, *linenr);
> +		return -128;
> +	}
> +	if (patch->is_delete > 0 && patch->new_name) {
> +		if (patch_input_file)
> +			error(_("git diff header for deleted file %s names a new file %s at %s:%d"),
> +			      patch->old_name, patch->new_name,
> +			      patch_input_file, *linenr);
> +		else
> +			error(_("git diff header for deleted file %s names a new file %s (line %d)"),
> +			      patch->old_name, patch->new_name, *linenr);
> +		return -128;
> +	}
>   	patch->is_toplevel_relative = 1;
>   	return offset;
>   }
> diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh
> index 751a01f92f..ff48e68da0 100755
> --- a/t/t4133-apply-filenames.sh
> +++ b/t/t4133-apply-filenames.sh
> @@ -146,4 +146,35 @@ test_expect_success 'name the empty "diff --git" line in the error' '
>   	reject_empty_git_header later_line.diff 3
>   '
>   
> +for old in /dev/null a/f
> +do
> +	test_expect_success "reject \"--- $old\" before \"new file mode\"" '
> +		cat >old_before_new.diff <<-EOF &&
> +		diff --git a/f b/f
> +		--- $old
> +		+++ b/f
> +		new file mode 100644
> +		@@ -0,0 +1 @@
> +		+1
> +		EOF
> +		test_must_fail git apply --check old_before_new.diff 2>err &&
> +		test_grep "header for new file f names an old file" err
> +	'
> +done
> +
> +test_expect_success 'reject "+++ /dev/null" before "deleted file mode"' '
> +	cat >new_before_delete.diff <<-\EOF &&
> +	diff --git a/f b/f
> +	--- a/f
> +	+++ /dev/null
> +	deleted file mode 100644
> +	@@ -1 +0,0 @@
> +	-1
> +	EOF
> +	test_must_fail git apply --check new_before_delete.diff 2>err &&
> +	test_grep "header for deleted file f names a new file" err &&
> +	test_must_fail git apply --check -R new_before_delete.diff 2>err &&
> +	test_grep "header for deleted file f names a new file" err
> +'
> +
>   test_done