git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[RFC PATCH 1/2] add-patch: compare object id instead of literal string

From
Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date
Jan 28, 2024, 18:11 UTC
Message-ID
<20240128181202.986753-3-shyamthakkar001@gmail.com>
In-Reply-To
<20240128181202.986753-2-shyamthakkar001@gmail.com>

Add a new function reveq(), which takes repository struct and two revision strings as arguments and returns 0 if the revisions point to the same object. Passing a rev which does not point to an object is considered undefined behavior as the underlying function memcmp() will be called with NULL hash strings.

Subsequently, replace literal string comparison to HEAD in run_add_p() with reveq() to handle more ways of saying HEAD (such as '@' or '$branch' where $branch points to same commit as HEAD). This addresses the NEEDSWORK comment in run_add_p().

However, in ADD_P_RESET mode keep string comparison in logical OR with reveq() to handle unborn HEAD.

As for the behavior change, with this patch applied if the given revision points to the same object as HEAD, the patch mode will be set to patch_mode_(reset,checkout,worktree)_head instead of patch_mode_(...)_nothead. That is equivalent of not setting -R flag in diff-index, which would have been otherwise set before this patch. However, when given same set of inputs, the actual outcome is same as before this patch. Therefore, this does not affect any automated scripts.

Also, add testcases to check the similarity of result between different ways of saying HEAD.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
Should the return values of repo_get_oid() be checked in reveq()? As
reveq() is not a global function and is only used in run_add_p(), the
validity of revisions is already checked beforehand by builtin/checkout.c
and builtin/reset.c before the call to run_add_p().
 add-patch.c               | 28 +++++++++++++++-------
 t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
 t/t2071-restore-patch.sh  | 21 ++++++++++------
 t/t7105-reset-patch.sh    | 14 +++++++++++
 4 files changed, 77 insertions(+), 36 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..01eb71d90e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -14,6 +14,7 @@
 #include "color.h"
 #include "compat/terminal.h"
 #include "prompt.h"
+#include "hash.h"
 
 enum prompt_mode_type {
 	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
@@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
 		     INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
 }
 
+// Check if two revisions point to the same object. Passing a rev which does not
+// point to an object is undefined behavior.
+static inline int reveq(struct repository *r, const char *rev1,
+			const char *rev2)
+{
+	struct object_id oid_rev1, oid_rev2;
+	repo_get_oid(r, rev1, &oid_rev1);
+	repo_get_oid(r, rev2, &oid_rev2);
+
+	return !oideq(&oid_rev1, &oid_rev2);
+}
+
 static int parse_range(const char **p,
 		       unsigned long *offset, unsigned long *count)
 {
@@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		s.mode = &patch_mode_stash;
 	else if (mode == ADD_P_RESET) {
 		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
+		 * The literal string comparison to HEAD below is kept
+		 * to handle unborn HEAD.
 		 */
-		if (!revision || !strcmp(revision, "HEAD"))
+		if (!revision || !strcmp(revision, "HEAD") ||
+		    !reveq(r, revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
 			s.mode = &patch_mode_reset_nothead;
 	} else if (mode == ADD_P_CHECKOUT) {
 		if (!revision)
 			s.mode = &patch_mode_checkout_index;
-		else if (!strcmp(revision, "HEAD"))
+		else if (!reveq(r, revision, "HEAD"))
 			s.mode = &patch_mode_checkout_head;
 		else
 			s.mode = &patch_mode_checkout_nothead;
 	} else if (mode == ADD_P_WORKTREE) {
 		if (!revision)
 			s.mode = &patch_mode_checkout_index;
-		else if (!strcmp(revision, "HEAD"))
+		else if (!reveq(r, revision, "HEAD"))
 			s.mode = &patch_mode_worktree_head;
 		else
 			s.mode = &patch_mode_worktree_nothead;
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..431f34fa9c 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -12,6 +12,7 @@ test_expect_success 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -38,26 +39,35 @@ test_expect_success 'git checkout -p with staged changes' '
 	verify_state dir/foo index index
 '
 
-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
-	set_and_save_state dir/foo work head &&
-	test_write_lines n y n | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_saved_state dir/foo
-'
-
-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
-	test_write_lines n y y | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head head
-'
-
-test_expect_success 'git checkout -p HEAD with change already staged' '
-	set_state dir/foo index index &&
-	# the third n is to get out in case it mistakenly does not apply
-	test_write_lines n y n | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head head
-'
+# Note: 'newbranch' points to the same commit as HEAD. And it is technically
+# allowed to name a branch '@' as of now, however in below test '@'
+# represents the shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+	test_expect_success "git checkout -p $opt with NO staged changes: abort" '
+		set_and_save_state dir/foo work head &&
+		test_write_lines n y n | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_saved_state dir/foo &&
+		test_grep "Discard" output
+	'
+
+	test_expect_success "git checkout -p $opt with NO staged changes: apply" '
+		test_write_lines n y y | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head head &&
+		test_grep "Discard" output
+	'
+
+	test_expect_success "git checkout -p $opt with change already staged" '
+		set_state dir/foo index index &&
+		# the third n is to get out in case it mistakenly does not apply
+		test_write_lines n y n | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head head &&
+		test_grep "Discard" output
+	'
+done
 
 test_expect_success 'git checkout -p HEAD^...' '
 	# the third n is to get out in case it mistakenly does not apply
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e..305b4a0c4f 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -12,6 +12,7 @@ test_expect_success PERL 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -44,13 +45,19 @@ test_expect_success PERL 'git restore -p with staged changes' '
 	verify_state dir/foo index index
 '
 
-test_expect_success PERL 'git restore -p --source=HEAD' '
-	set_state dir/foo work index &&
-	# the third n is to get out in case it mistakenly does not apply
-	test_write_lines n y n | git restore -p --source=HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head index
-'
+# Note: 'newbranch' points to the same commit as HEAD. And '@' is a
+# shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+	test_expect_success PERL "git restore -p --source=$opt" '
+		set_state dir/foo work index &&
+		# the third n is to get out in case it mistakenly does not apply
+		test_write_lines n y n | git restore -p --source=$opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head index &&
+		test_grep "Discard" output
+	'
+done
 
 test_expect_success PERL 'git restore -p --source=HEAD^' '
 	set_state dir/foo work index &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 05079c7246..65a8802b29 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -13,6 +13,7 @@ test_expect_success PERL 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -33,6 +34,19 @@ test_expect_success PERL 'git reset -p' '
 	test_grep "Unstage" output
 '
 
+# Note: '@' can technically also be used as a branch name, but in below test
+# it represents the shortcut for HEAD. And 'newbranch' points to the same
+# commit as HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+	test_expect_success PERL "git reset -p $opt" '
+		test_write_lines n y | git reset -p $opt >output &&
+		verify_state dir/foo work head &&
+		verify_saved_state bar &&
+		test_grep "Unstage" output
+	'
+done
+
 test_expect_success PERL 'git reset -p HEAD^' '
 	test_write_lines n y | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&

-- 
2.43.0
Previous: Ghanshyam ThakkarNext: Patrick Steinhardt
Message 2 of 46 in “add-patch: compare object id”
  1. Ghanshyam ThakkarJan 28, 2024
  2. 1/2 add-patch: compare object id instead of literal stringGhanshyam Thakkar, Jan 28, 2024
  3. Patrick SteinhardtJan 29, 2024
  4. Ghanshyam ThakkarJan 30, 2024
  5. Junio C HamanoJan 30, 2024
  6. Junio C HamanoJan 29, 2024
  7. Junio C HamanoJan 29, 2024
  8. Ghanshyam ThakkarJan 30, 2024
  9. 2/2 checkout: remove HEAD special caseGhanshyam Thakkar, Jan 28, 2024
  10. Patrick SteinhardtJan 29, 2024
  11. 0/2 add-patch: Support '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 2, 2024
  12. 1/2 add-patch: remove non-relevant NEEDSWORK commentGhanshyam Thakkar, Feb 2, 2024
  13. 2/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 2, 2024
  14. Junio C HamanoFeb 2, 2024
  15. Junio C HamanoFeb 2, 2024
  16. Ghanshyam ThakkarFeb 2, 2024
  17. Ghanshyam ThakkarFeb 2, 2024
  18. Ghanshyam ThakkarFeb 2, 2024
  19. 0/2 add-patch: '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 3, 2024
  20. 0/3 '@' as a synonym for 'HEAD' in patch modeGhanshyam Thakkar, Feb 6, 2024
  21. 0/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 11, 2024
  22. 0/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 13, 2024
  23. Phillip WoodFeb 14, 2024
  24. 1/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 13, 2024
  25. 2/2 add -p tests: remove PERL prerequisitesGhanshyam Thakkar, Feb 13, 2024
  26. 1/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 11, 2024
  27. Junio C HamanoFeb 12, 2024
  28. 2/2 add -p tests: remove PERL prerequisitesGhanshyam Thakkar, Feb 11, 2024
  29. 1/3 add-patch: remove unnecessary NEEDSWORK commentGhanshyam Thakkar, Feb 6, 2024
  30. Phillip WoodFeb 7, 2024
  31. 2/3 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 6, 2024
  32. Junio C HamanoFeb 7, 2024
  33. Phillip WoodFeb 7, 2024
  34. Ghanshyam ThakkarFeb 9, 2024
  35. 3/3 add -p tests: remove Perl prerequisiteGhanshyam Thakkar, Feb 6, 2024
  36. Phillip WoodFeb 7, 2024
  37. Phillip WoodFeb 7, 2024
  38. Junio C HamanoFeb 7, 2024
  39. Eric SunshineFeb 7, 2024
  40. 1/2 add-patch: remove unnecessary NEEDSWORK commentGhanshyam Thakkar, Feb 3, 2024
  41. 2/2 add-patch: classify '@' as a synonym for 'HEAD'Ghanshyam Thakkar, Feb 3, 2024
  42. Junio C HamanoFeb 3, 2024
  43. Ghanshyam ThakkarFeb 5, 2024
  44. Phillip WoodFeb 5, 2024
  45. Ghanshyam ThakkarFeb 5, 2024
  46. Junio C HamanoFeb 5, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.