Advice on checkout dirty files

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

Documentation entry in Documentation/config/advice.txt

This is my submission for microproject [GSOC]

This patch adds a new advice type ADVICE_STASH_BEFORE_CHECKOUT to help users when they attempt to switch branches with local modifications that would be overwritten by the operation.

The new advice follows the same patterns established by existing advice functions such as advise_on_updating_sparse_paths(). When triggered, it lists the affected files and suggests using git stash push/pop to save and restore local changes.

The advice can be silenced with:

git config set advice.stashBeforeCheckout false

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Arsh Srivastava (1):
advice: add stashBeforeCheckout advice for dirty branch switches

Junio C Hamano (1):
The 13th batch

K Jayatheerth (1):
repo: remove unnecessary variable shadow

LorenzoPegorari (2):
diff: handle ANSI escape codes in prefix when calculating diffstat
width
t4052: test for diffstat width when prefix contains ANSI escape codes

  Documentation/RelNotes/2.54.0.adoc | 14 +++++++++++++
  Documentation/config/advice.adoc   |  5 +++++
  advice.c                           | 27 +++++++++++++++++++++++++
  advice.h                           |  2 ++
  builtin/repo.c                     |  1 -
  diff.c                             | 12 ++++-------
  t/t4052-stat-output.sh             | 32 ++++++++++++++++++++++++++++++
  7 files changed, 84 insertions(+), 9 deletions(-)

base-commit: 3fe08b8fd1f7731edabeab8138547ec88d6407de
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2233%2FArsh123344423%2Fadvice_on_checkout_dirty_files-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2233/Arsh123344423/advice_on_checkout_dirty_files-v1
Pull-Request: https://github.com/git/git/pull/2233

rebase my file’s pointer head

Hi Arsh

Quoted 5 lines · Arsh Srivastava via GitGitGadget

This is my submission for microproject [GSOC]

This patch adds a new advice type ADVICE_STASH_BEFORE_CHECKOUT to help users when they attempt to switch branches with local modifications that would be overwritten by the operation.

If the intent is for the user to carry over the changes to the new branch then recommending "git checkout -m" might be more convenient rather than having to stash, checkout and unstash as three separate steps.

Something seems to have gone awry with your branch as there are other patches in this series. You should rebase your branch onto the upstream master branch with

     git rebase --onto origin/master HEAD^

and then when you push it double check how many commits there are in the summary of the pull request before submitting.

Thanks

Phillip

Quoted 46 lines

The new advice follows the same patterns established by existing advice functions such as advise_on_updating_sparse_paths(). When triggered, it lists the affected files and suggests using git stash push/pop to save and restore local changes.

The advice can be silenced with:

git config set advice.stashBeforeCheckout false

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Arsh Srivastava (1):
advice: add stashBeforeCheckout advice for dirty branch switches

Junio C Hamano (1):
The 13th batch

K Jayatheerth (1):
repo: remove unnecessary variable shadow

LorenzoPegorari (2):
diff: handle ANSI escape codes in prefix when calculating diffstat
width
t4052: test for diffstat width when prefix contains ANSI escape codes

  Documentation/RelNotes/2.54.0.adoc | 14 +++++++++++++
  Documentation/config/advice.adoc   |  5 +++++
  advice.c                           | 27 +++++++++++++++++++++++++
  advice.h                           |  2 ++
  builtin/repo.c                     |  1 -
  diff.c                             | 12 ++++-------
  t/t4052-stat-output.sh             | 32 ++++++++++++++++++++++++++++++
  7 files changed, 84 insertions(+), 9 deletions(-)

base-commit: 3fe08b8fd1f7731edabeab8138547ec88d6407de
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2233%2FArsh123344423%2Fadvice_on_checkout_dirty_files-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2233/Arsh123344423/advice_on_checkout_dirty_files-v1
Pull-Request: https://github.com/git/git/pull/2233

If the intent is for the user to carry over the changes to the new branch then recommending "git checkout -m" might be more convenient rather than having to stash, checkout and unstash as three separate steps.

Quoted 3 lines · Phillip Wood

If the intent is for the user to carry over the changes to the new branch then recommending "git checkout -m" might be more convenient rather than having to stash, checkout and unstash as three separate steps.

I personally would not recommend pushing "-m" to new people without explaining its ramifications, though.

If "git stash pop" fails while a commit different from the original is checked out, the working tree will get conflicts for you to resolve, and that is the same as "git checkout -m". But the conflict may turn out to be too complex that you might not be able to cleanly resolve.

With a "git stash pop" that gets interrupted by a conflict, the stash entry is not removed from the stash, so there is a clean recourse to "git reset --hard" away the conflict and attempting to unstash (either to the same commit or to a different base).

With "git checkout -m", on the other hand, there is no such recourse. The conflicted working tree with the unmerged index is all you get, and you get only a single chance to resolve it correctly.

So...

As per the recommendation of Phillip Wood <[email protected]> I have changed my files and added git checkout -m after understanding its significance :)

It is understood as author that I understand.
It is the users who need to understand.

Quoted 3 lines · Arsh Srivastava

As per the recommendation of Phillip Wood <[email protected]> I have changed my files and added git checkout -m after understanding its significance :)

It is a given that you as the author of the patch understand it. What I meant was that the users who get the recommendation need to be told about the potential issues.

It is understood as author that I understand.
It is the users who need to understand.
Potential risks for the user.

Quoted 3 lines · Arsh Srivastava

As per the recommendation of Phillip Wood <[email protected]> I have changed my files and added git checkout -m after understanding its significance :)

It is a given that you as the author of the patch understand it. What I meant was that the users who get the recommendation need to be told about the potential issues.

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

Documentation entry in Documentation/config/advice.txt

Rebased my files pointer
Changed advice to git checkout -m

I have as you mentioned changed

Quoted 2 lines

Rebased my files pointer
Changed advice to git checkout -m

Thank you

Signed-off-by Arsh Srivastava <[email protected]>

Quoted 146 lines · Arsh Srivastava via GitGitGadget

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

The advice follows existing patterns established by advise_on_updating_sparse_paths() and can be silenced with:

git config set advice.stashBeforeCheckout false

Signed-off-by Arsh Srivastava <[email protected]>

---
Advice on checkout dirty files

This is my submission for microproject [GSOC]

This patch adds a new advice type ADVICE_STASH_BEFORE_CHECKOUT to help users when they attempt to switch branches with local modifications that would be overwritten by the operation.

The new advice follows the same patterns established by existing advice functions such as advise_on_updating_sparse_paths(). When triggered, it lists the affected files and suggests using git stash push/pop to save and restore local changes.

The advice can be silenced with:

git config set advice.stashBeforeCheckout false

Changes:

> advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add > "stashBeforeCheckout" to advice_setting[] and implement > advise_on_checkout_dirty_files() function
> Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2233%2FArsh123344423%2Fadvice_on_checkout_dirty_files-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2233/Arsh123344423/advice_on_checkout_dirty_files-v2
Pull-Request: https://github.com/git/git/pull/2233

Range-diff vs v1:

 1:  0ed992956e < -:  ---------- diff: handle ANSI escape codes in prefix when calculating diffstat width
 2:  c70043a2c0 < -:  ---------- t4052: test for diffstat width when prefix contains ANSI escape codes
 3:  185356a454 < -:  ---------- repo: remove unnecessary variable shadow
 4:  acebdd714b < -:  ---------- The 13th batch
 5:  9ec447e3cb = 1:  eb5639dbc3 advice: add stashBeforeCheckout advice for dirty branch switches

 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Documentation/config/advice.adoc +5 -0

index 257db58918..8752e05636 100644
@@ -126,6 +126,11 @@ all advice messages.
                Shown when a sparse index is expanded to a full index, which is likely
                due to an unexpected set of files existing outside of the
                sparse-checkout.
+       stashBeforeCheckout::
+               Shown when the user attempts to switch branches but has
+               local modifications that would be overwritten by the
+               operation, to suggest using linkgit:git-stash[1] to
+               save changes before switching.
        statusAheadBehind::
                Shown when linkgit:git-status[1] computes the ahead/behind
                counts for a local ref compared to its remote tracking ref,

advice.c +27 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
        [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
        [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
        [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+       [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
        [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
        [ADVICE_STATUS_HINTS]                           = { "statusHints" },
        [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                            "* Use \"git add --sparse <paths>\" to update the index\n"
                            "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+       return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+                     "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+       fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+                     _("You can save your local changes before switching by running:\n"
+                       "\n"
+                       "\tgit stash push\n"
+                       "\n"
+                       "Then restore them after switching with:\n"
+                       "\n"
+                       "\tgit stash pop\n"
+                       "\n"
+                       "Or to discard your local changes, use:\n"
+                       "\n"
+                       "\tgit checkout -- <file>"));
+}

advice.h +2 -0

index 8def280688..c035b5d8e3 100644
@@ -48,6 +48,7 @@ enum advice_type {
        ADVICE_SET_UPSTREAM_FAILURE,
        ADVICE_SKIPPED_CHERRY_PICKS,
        ADVICE_SPARSE_INDEX_EXPANDED,
+       ADVICE_STASH_BEFORE_CHECKOUT,
        ADVICE_STATUS_AHEAD_BEHIND_WARNING,
        ADVICE_STATUS_HINTS,
        ADVICE_STATUS_U_OPTION,

@@ -83,5 +84,6 @@ void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 void advise_on_moving_dirty_path(struct string_list *pathspec_list);
+void advise_on_checkout_dirty_files(struct string_list *file_list);
 
 #endif /* ADVICE_H */

base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2 --
gitgitgadget

advice.c +1 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
 	[ADVICE_SPARSE_INDEX_EXPANDED]			= { "sparseIndexExpanded" },
+	[ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
 	[ADVICE_STATUS_HINTS]				= { "statusHints" },
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption" },

Quoted 13 lines · Arsh Srivastava via GitGitGadget

advice.c +1 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
      [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
      [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
      [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+     [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
      [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
      [ADVICE_STATUS_HINTS]                           = { "statusHints" },
      [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

advice.c void advise_on_moving_dirty_path(struct string_list *pathspec_list) · line 317

Quoted 5 lines

advice.c +2 -0

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                          "* Use \"git add --sparse <paths>\" to update the index\n"
                          "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)

Huh. So this patch wires up a new function and advice, but we don't ever seem to use it. Am I missing something?

Patrick

Quoted 14 lines · Patrick Steinhardt

Quoted 13 lines · Arsh Srivastava via GitGitGadget

advice.c +1 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
      [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
      [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
      [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+     [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
      [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
      [ADVICE_STATUS_HINTS]                           = { "statusHints" },
      [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)

Quoted 8 lines

advice.c void advise_on_moving_dirty_path(struct string_list *pathspec_list) · line 317

Quoted 5 lines

advice.c +2 -0

                          "* Use \"git add --sparse <paths>\" to update the index\n"
                          "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)

Huh. So this patch wires up a new function and advice, but we don't ever seem to use it. Am I missing something?

Thank you so much for looking into my PR and i believe advice.h is used in the add.c file. And advice really helps young developers understand what's wrong in their files because navigating git and trying to find solutions is very difficult, causing them to go to ai models making them copy pasting machines.

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

Documentation entry in Documentation/config/advice.txt

advice.h is used in the add.c file.
And advice really helps young developers

causing them to go to ai models making them copy pasting machines.

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

The advice follows existing patterns established by advise_on_updating_sparse_paths() and can be silenced with:

git config set advice.stashBeforeCheckout false

Signed-off-by Arsh Srivastava <[email protected]>

---
 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Documentation/config/advice.adoc +5 -0

index 257db58918..8752e05636 100644
@@ -126,6 +126,11 @@ all advice messages.
                Shown when a sparse index is expanded to a full index, which is likely
                due to an unexpected set of files existing outside of the
                sparse-checkout.
+       stashBeforeCheckout::
+               Shown when the user attempts to switch branches but has
+               local modifications that would be overwritten by the
+               operation, to suggest using linkgit:git-stash[1] to
+               save changes before switching.
        statusAheadBehind::
                Shown when linkgit:git-status[1] computes the ahead/behind
                counts for a local ref compared to its remote tracking ref,

advice.c +27 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
        [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
        [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
        [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+       [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
        [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
        [ADVICE_STATUS_HINTS]                           = { "statusHints" },
        [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                            "* Use \"git add --sparse <paths>\" to update the index\n"
                            "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+       return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+                     "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+       fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+                     _("You can save your local changes before switching by running:\n"
+                       "\n"
+                       "\tgit stash push\n"
+                       "\n"
+                       "Then restore them after switching with:\n"
+                       "\n"
+                       "\tgit stash pop\n"
+                       "\n"
+                       "Or to discard your local changes, use:\n"
+                       "\n"
+                       "\tgit checkout -- <file>"));
+}

advice.h +2 -1

index 8def280688..c035b5d8e3 100644
@@ -48,6 +48,7 @@ enum advice_type {
        ADVICE_SET_UPSTREAM_FAILURE,
        ADVICE_SKIPPED_CHERRY_PICKS,
        ADVICE_SPARSE_INDEX_EXPANDED,
+       ADVICE_STASH_BEFORE_CHECKOUT,
        ADVICE_STATUS_AHEAD_BEHIND_WARNING,
        ADVICE_STATUS_HINTS,
        ADVICE_STATUS_U_OPTION,

@@ -83,5 +84,6 @@ void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 void advise_on_moving_dirty_path(struct string_list *pathspec_list);
+void advise_on_checkout_dirty_files(struct string_list *file_list);
 
 #endif /* ADVICE_H */
--

gitgitgadget

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

Documentation entry in Documentation/config/advice.txt

This is my submission for microproject [GSOC]

This patch adds a new advice type ADVICE_STASH_BEFORE_CHECKOUT to help users when they attempt to switch branches with local modifications that would be overwritten by the operation.

The new advice follows the same patterns established by existing advice functions such as advise_on_updating_sparse_paths(). When triggered, it lists the affected files and suggests using git stash push/pop to save and restore local changes.

The advice can be silenced with:

git config set advice.stashBeforeCheckout false

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Arsh Srivastava (5):
advice: add stashBeforeCheckout advice for dirty branch switches
advice: add stashBeforeCheckout advice for dirty branch switches

    [GSOC]

unpack-trees: suggesting 'git checkout -m <branch>' with its
repercussions
Updating tests and unpack-tress.c [GSOC]
File updation [GSOC]

 t/t6439-merge-co-error-msgs.sh | 6 ++++++
 t/t7406-submodule-update.sh    | 3 +++
 unpack-trees.c                 | 9 +++++++--
 3 files changed, 16 insertions(+), 2 deletions(-)

base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2233%2FArsh123344423%2Fadvice_on_checkout_dirty_files-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2233/Arsh123344423/advice_on_checkout_dirty_files-v4
Pull-Request: https://github.com/git/git/pull/2233

Range-diff vs v3:

 1:  eb5639dbc3 = 1:  eb5639dbc3 advice: add stashBeforeCheckout advice for dirty branch switches
 2:  e88c851701 = 2:  e88c851701 advice: add stashBeforeCheckout advice for dirty branch switches [GSOC]
 -:  ---------- > 3:  4237b9667d unpack-trees: suggesting 'git checkout -m <branch>' with its repercussions
 -:  ---------- > 4:  b25ea22410 Updating tests and unpack-tress.c [GSOC]
 -:  ---------- > 5:  2ef7d5a3d6 File updation [GSOC]

provide perfect logic not drunken-man's-walk series.
not a place for you to show how you made wrong turns before arriving at the final shape of the code. history for later developers to see in "git log" output to learn from
what is "updation"

Quoted 15 lines · Arsh Srivastava via GitGitGadget

This is my submission for microproject [GSOC]

This patch adds a new advice type ADVICE_STASH_BEFORE_CHECKOUT to help users when they attempt to switch branches with local modifications that would be overwritten by the operation.

The new advice follows the same patterns established by existing advice functions such as advise_on_updating_sparse_paths(). When triggered, it lists the affected files and suggests using git stash push/pop to save and restore local changes.

The advice can be silenced with:

git config set advice.stashBeforeCheckout false

Quoted 8 lines

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Even though no developer is perfect, when you are presenting your updated work, armed with wisdom borrowed from your reviewers' comments on your earlier attempts, you are expected to take the opportunity to pretend to have written a series of patches that are perfect logical progression towards the final shape of the code without detours, change of plans, and fixing earlier mistakes made in the series.

Please do not throw a drunken-man's-walk series at us. For example, I see that [PATCH 3/5] literally removes what was added by earlier patches. This is not a place for you to show how you made wrong turns before arriving at the final shape of the code.

The final series accepted by the project will have to stay in our history for later developers to see in "git log" output to learn from, and a series being clean logical progression is a must for that to happen.

Also, what is "updation"? Is it a standard English word, or some dialect of an LLM origin?

Quoted 27 lines

Arsh Srivastava (5):
advice: add stashBeforeCheckout advice for dirty branch switches
advice: add stashBeforeCheckout advice for dirty branch switches

    [GSOC]

unpack-trees: suggesting 'git checkout -m <branch>' with its
repercussions
Updating tests and unpack-tress.c [GSOC]
File updation [GSOC]

 t/t6439-merge-co-error-msgs.sh | 6 ++++++
 t/t7406-submodule-update.sh    | 3 +++
 unpack-trees.c                 | 9 +++++++--
 3 files changed, 16 insertions(+), 2 deletions(-)

base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2233%2FArsh123344423%2Fadvice_on_checkout_dirty_files-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2233/Arsh123344423/advice_on_checkout_dirty_files-v4
Pull-Request: https://github.com/git/git/pull/2233

Range-diff vs v3:

 1:  eb5639dbc3 = 1:  eb5639dbc3 advice: add stashBeforeCheckout advice for dirty branch switches
 2:  e88c851701 = 2:  e88c851701 advice: add stashBeforeCheckout advice for dirty branch switches [GSOC]
 -:  ---------- > 3:  4237b9667d unpack-trees: suggesting 'git checkout -m <branch>' with its repercussions
 -:  ---------- > 4:  b25ea22410 Updating tests and unpack-tress.c [GSOC]
 -:  ---------- > 5:  2ef7d5a3d6 File updation [GSOC]

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

Documentation entry in Documentation/config/advice.txt

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Signed-off-by Arsh Srivastava [email protected]

Arsh Srivastava (3):
advice: add stashBeforeCheckout advice for dirty branch switches
advice: add stashBeforeCheckout advice for dirty branch switches

    [GSOC]

unpack-trees: suggesting 'git checkout -m <branch>' with its
repercussions

He still see 3 patches for a work that can be done by one. That is what "perfect logical progression without detours, change of plans, and fixes earlier mistake" is about.
please do not throw a drunken-man's-walk series at
us. Pretend to be a perfect developer

Quoted 7 lines · Arsh Srivastava via GitGitGadget

Changes:

Quoted 4 lines

advice.h: add ADVICE_STASH_BEFORE_CHECKOUT enum value advice.c: add "stashBeforeCheckout" to advice_setting[] and implement advise_on_checkout_dirty_files() function Documentation/config/advice.adoc: document the new advice key

Quoted 8 lines

Signed-off-by Arsh Srivastava [email protected]

Arsh Srivastava (3):
advice: add stashBeforeCheckout advice for dirty branch switches
advice: add stashBeforeCheckout advice for dirty branch switches

    [GSOC]

unpack-trees: suggesting 'git checkout -m <branch>' with its
repercussions

I still see that [PATCH 3/3] literally removes what was added by earlier patches. ADVICE_STASH_BEFORE_CHECKOUT is added to advice.[ch] and stashBeforeCheckOut is added to Documentation/config/adivce.adoc in [1/3], and then they are removed in [3/3]. If your final solution does not involve such an advice, then do not even add it in an earlier patch, only to retract it and replace it with something else in a later patch. If the reason why that "something else" replaces the advice is because it is a better solution to the problem you initially started to solve than the advice message added in [1/3], then just go straight to that "something else", without adding and removing the advice mechanism.

That is what "perfect logical progression without detours, change of plans, and fixes earlier mistake" is about.

In other words, please do not throw a drunken-man's-walk series at us. Pretend to be a perfect developer.

The final series accepted by the project will have to stay in our history for later developers to see in "git log" output to learn from, and a series being clean logical progression is a must for that to happen.

From: Arsh Srivastava <[email protected]>

This comment is an extention to the already existing stash comment. Added updated comment over the already existing function "setup_unpack_trees_porcelain" with "git checkout -m" and its repercussions
I have also mentioned the repercussions of using "-m".

Signed-off-by Arsh Srivastava [email protected]

Try using 'git checkout -m <branch>' for a quick fix. Please Note :- that using -m (merge) will not save your changes, rather would directly merge them.
Meaning if you are not able to resolve conflicts and does --hard reset your local changes would be gone.

The commit message should focus on the "why" and "what" from a user perspective, following the project's standard format (problem description, then solution).
Also showed an example for the same.
Also, note that "extention" is a typo; it should be "extension".
Having said that, I am not sure if we want to suggest "checkout -m" in this situation after all.
Pointed out the difference between "stash" and "checkout -m".
When advice requires a multi-line warning about potential data loss.
The goal of these messages should be to provide a clear, safe next step, not a list of advanced alternatives with caveats.
After all, the users who need such an "it failed, now what should I do to recover?" message the most are relatively inexperienced users and we do not want the advice to be overwhelming.
Suggesting this "one-shot" approach to a user who is already in a state of friction (and likely less experienced) might be providing them with a "foot-gun" rather than a helpful tip. Generally, advice that nudges users toward the safest "golden paths" like stashing or committing is preferred. For a microproject, you've successfully demonstrated that you can modify the advice system and update the test suite.
it might be better to drop the 'checkout -m' suggestion and instead focus on making the existing 'stash' and 'commit' advice as clear and helpful as possible.

Quoted 9 lines · Arsh Srivastava via GitGitGadget

From: Arsh Srivastava <[email protected]>

This comment is an extention to the already existing stash comment. Added updated comment over the already existing function "setup_unpack_trees_porcelain" with "git checkout -m" and its repercussions
I have also mentioned the repercussions of using "-m".

Signed-off-by Arsh Srivastava [email protected]

The commit message should focus on the "why" and "what" from a user perspective, following the project's standard format (problem description, then solution).

Consider a more standard phrasing:

unpack-trees: suggest 'git checkout -m' when checkout fails

When a branch switch fails due to local changes, we suggest stashing or committing. However, 'git checkout -m' is a valid alternative for users who wish to carry their changes over via a merge.

Update the advice message to suggest this option, while including a warning about the potential for data loss if a hard reset is performed after a conflicted merge.

Also, note that "extention" is a typo; it should be "extension".

Having said that, I am not sure if we want to suggest "checkout -m" in this situation after all.

The added message is quite long:

Quoted 5 lines

Try using 'git checkout -m <branch>' for a quick fix. Please Note :- that using -m (merge) will not save your changes, rather would directly merge them.
Meaning if you are not able to resolve conflicts and does --hard reset your local changes would be gone.

When advice requires a multi-line warning about potential data loss, it's often a sign that the operation being suggested isn't suitable for general advice. The goal of these messages should be to provide a clear, safe next step, not a list of advanced alternatives with caveats. After all, the users who need such an "it failed, now what should I do to recover?" message the most are relatively inexperienced users and we do not want the advice to be overwhelming.

The primary concern is that 'git checkout -m' is a high-stakes operation compared to 'git stash'.

- When a user uses 'git stash', their changes are recorded in a stash entry. If 'git stash pop' later results in conflicts they cannot resolve, the user can always 'git reset --hard' to get back to a clean state, knowing their original changes are still safe in the stash entry, which they can re-attempt to use later.

- In contrast, 'git checkout -m' performs the merge directly in the working tree. If conflicts arise, the original local changes are immediately replaced by conflict markers. Unlike stash, there is no "undo" record. If the user realizes they are in over their head, they cannot simply "abort" to get their original changes back. They have only one chance to resolve it correctly, and they have to do so right there.

Suggesting this "one-shot" approach to a user who is already in a state of friction (and likely less experienced) might be providing them with a "foot-gun" rather than a helpful tip. Generally, advice that nudges users toward the safest "golden paths" like stashing or committing is preferred.

For a microproject, you've successfully demonstrated that you can modify the advice system and update the test suite. However, for the health of the project's usability, it might be better to drop the 'checkout -m' suggestion and instead focus on making the existing 'stash' and 'commit' advice as clear and helpful as possible.

Thanks.

Junio C Hamano <[email protected]> writes :

Quoted 11 lines

The commit message should focus on the "why" and "what" from a user perspective, following the project's standard format (problem description, then solution).
Also showed an example for the same.
Also, note that "extention" is a typo; it should be "extension".
Having said that, I am not sure if we want to suggest "checkout -m" in this situation after all.
Pointed out the difference between "stash" and "checkout -m".
When advice requires a multi-line warning about potential data loss.
The goal of these messages should be to provide a clear, safe next step, not a list of advanced alternatives with caveats.
After all, the users who need such an "it failed, now what should I do to recover?" message the most are relatively inexperienced users and we do not want the advice to be overwhelming.
Suggesting this "one-shot" approach to a user who is already in a state of friction (and likely less experienced) might be providing them with a "foot-gun" rather than a helpful tip. Generally, advice that nudges users toward the safest "golden paths" like stashing or committing is preferred. For a microproject, you've successfully demonstrated that you can modify the advice system and update the test suite.
it might be better to drop the 'checkout -m' suggestion and instead focus on making the existing 'stash' and 'commit' advice as clear and helpful as possible.

I wonder where this came from, as it is quite unusual to have a rephrased summary of what you respond to. Is this LLM-generated summary that was copied-and-pasted without much human brain effort? What is more usual is to quote the message you are responding to.

Quoted 12 lines · Arsh Srivastava

Junio C Hamano <[email protected]> writes :

Quoted 11 lines

The commit message should focus on the "why" and "what" from a user perspective, following the project's standard format (problem description, then solution).
Also showed an example for the same.
Also, note that "extention" is a typo; it should be "extension".
Having said that, I am not sure if we want to suggest "checkout -m" in this situation after all.
Pointed out the difference between "stash" and "checkout -m".
When advice requires a multi-line warning about potential data loss.
The goal of these messages should be to provide a clear, safe next step, not a list of advanced alternatives with caveats.
After all, the users who need such an "it failed, now what should I do to recover?" message the most are relatively inexperienced users and we do not want the advice to be overwhelming.
Suggesting this "one-shot" approach to a user who is already in a state of friction (and likely less experienced) might be providing them with a "foot-gun" rather than a helpful tip. Generally, advice that nudges users toward the safest "golden paths" like stashing or committing is preferred. For a microproject, you've successfully demonstrated that you can modify the advice system and update the test suite.
it might be better to drop the 'checkout -m' suggestion and instead focus on making the existing 'stash' and 'commit' advice as clear and helpful as possible.

I wonder where this came from, as it is quite unusual to have a rephrased summary of what you respond to. Is this LLM-generated summary that was copied-and-pasted without much human brain effort?

What is more usual is to quote the message you are responding to, trim the parts you are not going to comment on and not necessary for bystanders to read in order to understand your response, and then sprinkle your comments in between the parts of the quoted message, instead of saying only your thing before the message you are responding to without trimming (which is highly frowned upon, and people often tell you not to "top-post").

Junio C Hamano <[email protected]> write:

Quoted 3 lines

I wonder where this came from, as it is quite unusual to have a rephrased summary of what you respond to. Is this LLM-generated summary that was copied-and-pasted without much human brain effort? What is more usual is to quote the message you are responding to.

Actually as suspicious as it looks the mail was written by me only and I thought that each line was as important as other and
I wanted to reply to each point you mentioned
Sorry for shortening your response
next time I will make sure that I will make quotations direct.

That wasn't what I meant, and you are still top-posting X-<.

Quoted 10 lines · Arsh Srivastava

Junio C Hamano <[email protected]> write:

Quoted 3 lines

I wonder where this came from, as it is quite unusual to have a rephrased summary of what you respond to. Is this LLM-generated summary that was copied-and-pasted without much human brain effort? What is more usual is to quote the message you are responding to.

Actually as suspicious as it looks the mail was written by me only and I thought that each line was as important as other and
I wanted to reply to each point you mentioned
Sorry for shortening your response
next time I will make sure that I will make quotations direct.

That wasn't what I meant, and you are still top-posting X-<.

When a branch switch fails due to local changes and
new users who are not familiar with the error message often get confused about how to move ahead and resolve the issue as the previous error message only suggests to commit or stash the changes but doesn't explain how to do that or what the next steps are.

This patch enhances the error message with more specific instructions in a concise manner to help users understand how to resolve the issue and move their local changes safely to the other branch using stash.

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence. Perhaps the early part can become a bit easier to read with

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is.

You may view it as a weakness (which
may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Also "To move you" -> "To move your".

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Quoted 5 lines · Arsh Srivastava via GitGitGadget

When a branch switch fails due to local changes and
new users who are not familiar with the error message often get confused about how to move ahead and resolve the issue as the previous error message only suggests to commit or stash the changes but doesn't explain how to do that or what the next steps are.

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence.

Perhaps the early part can become a bit easier to read with punctuations, and cutting the sentence into two, e.g.,

When a branch switch fails due to local changes, new users who are unfamiliar with the error message often get confused about how to move ahead and resolve the issue.

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is. The _current_ message stops at hinting the commands to be used without giving wordy instructions that are best left to manuals. You may view it as a weakness (which may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

Quoted 4 lines

This patch enhances the error message with more specific instructions in a concise manner to help users understand how to resolve the issue and move their local changes safely to the other branch using stash.

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

By the way, the updated message seems much less concise than the original.

unpack-trees.c void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, · line 135

Quoted 9 lines

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"

unpack-trees.c +5 -2

-               "Please commit your changes or stash them before you switch branches.")
-           : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
+               "To move you local changes safely to the other branch,\n"
+               "Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge."
+               )
+           : _("Your local changes to the following files would be overwritten by checkout:\n%%s"
+               "Please commit your changes or stash them before you switch branches.");

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

The original was this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.")
: _("Your local changes to the following files would be overwritten by checkout:\n%%s");

Note that when advice is *NOT* enabled, we only gave

_("Your local changes to the following files would be overwritten by checkout:\n%%s");

without any "advise" in the output. That is what !advice_enabled() means.

The updated code does this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"To move you local changes safely to the other branch,\n"
"Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge." )
: _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.");

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Also "To move you" -> "To move your".

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

A more concise version might say:

Try 'git stash && git checkout <branch> && git stash pop' to carry your changes to the new branch, or commit your work before switching.

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thanks.

Junio C Hamano <[email protected]> writes :

Quoted 3 lines

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence. Perhaps the early part can become a bit easier to read with

punctuations, and cutting the sentence into two, e.g.,

In my future commits I will remember to make it as easy to read as possible. With less punctuations and shorter sentences which will in turn make it more concise.

Quoted 3 lines

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is.

Understood I will in future not use previous because it is the _current_ code.

Quoted 4 lines

You may view it as a weakness (which
may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

I understand, git wants people to not explore the available change options and help them make logical decisions rather than pushing them with some unneeded commands.

Quoted 3 lines

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

Understood that makes sense because nevertheless
it is given that I am writing the changes for this patch only.

Quoted 3 lines

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

That makes total sense.

Quoted 3 lines

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Thank you so much for your guidance the advice should not trigger to those who have opted not to see.
My code might have misjudged this paradigm.

Quoted 1 line

Also "To move you" -> "To move your".

I thought I had fixed this typo. Seems like I didn't. I will remember to be more cautious next time.

Quoted 3 lines

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Absolutely correct this commit does narrow the users vision for exploring.

Quoted 3 lines

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

I don't want to sound like a programmed robot but I absolutely agree with the recommendations.

Quoted 2 lines

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thank you so much for your valuable guidance.
If it's possible I want some guidance over the questions written below, As it is well stated by you that the current
text is clear enough.
Should I still work on this PR from a purely GSoC perspective. Or should I start making my proposal or
still work on this PR until my micro project is merged? Because I have already shown I can navigate git project which was the goal of micro projects in the first place.

We don’t require that your patch be accepted into the “master” branch by the time of your formal application

we mostly want to see that you have a basic level of competence and especially the
ability to interact with the other Git developers.

As such and seeing Junio's previous response, I would say that no further work is required here.

Quoted 70 lines · Arsh Srivastava

Junio C Hamano <[email protected]> writes :

Quoted 3 lines

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence. Perhaps the early part can become a bit easier to read with

punctuations, and cutting the sentence into two, e.g.,

In my future commits I will remember to make it as easy to read as possible. With less punctuations and shorter sentences which will in turn make it more concise.

Quoted 3 lines

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is.

Understood I will in future not use previous because it is the _current_ code.

Quoted 4 lines

You may view it as a weakness (which
may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

I understand, git wants people to not explore the available change options and help them make logical decisions rather than pushing them with some unneeded commands.

Quoted 3 lines

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

Understood that makes sense because nevertheless
it is given that I am writing the changes for this patch only.

Quoted 3 lines

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

That makes total sense.

Quoted 3 lines

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Thank you so much for your guidance the advice should not trigger to those who have opted not to see.
My code might have misjudged this paradigm.

Quoted 1 line

Also "To move you" -> "To move your".

I thought I had fixed this typo. Seems like I didn't. I will remember to be more cautious next time.

Quoted 3 lines

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Absolutely correct this commit does narrow the users vision for exploring.

Quoted 3 lines

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

I don't want to sound like a programmed robot but I absolutely agree with the recommendations.

Quoted 2 lines

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Quoted 10 lines

Thank you so much for your valuable guidance.
If it's possible I want some guidance over the questions written below, As it is well stated by you that the current
text is clear enough.
Should I still work on this PR from a purely GSoC perspective. Or should I start making my proposal or
still work on this PR until my micro project is merged? Because I have already shown I can navigate git project which was the goal of micro projects in the first place.

From the micro-project information [1] for GSoC we have:

The coding part of the microproject should be very small (say, 10-30 minutes). We don’t require that your patch be accepted into the “master” branch by the time of your formal application; we mostly want to see that you have a basic level of competence and especially the ability to interact with the other Git developers.

As such and seeing Junio's previous response, I would say that no further work is required here.

[1]: https://git.github.io/General-Microproject-Information/

[snip]

From the micro-project information [1] for GSoC we have:

The coding part of the microproject should be very small (say, 10-30 minutes). We don’t require that your patch be accepted into the “master” branch by the time of your formal application; we mostly want to see that you have a basic level of competence and especially the ability to interact with the other Git developers.

As such and seeing Junio's previous response, I would say that no further work is required here.

[1]: https://git.github.io/General-Microproject-Information/

I understand, git wants people to not explore the available change options and help them make logical decisions rather than pushing them with some unneeded commands.

Junio C Hamano <[email protected]> writes :

Quoted 3 lines

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence. Perhaps the early part can become a bit easier to read with

punctuations, and cutting the sentence into two, e.g.,

In my future commits I will remember to make it as easy to read as possible. With less punctuations and shorter sentences which will in turn make it more concise.

Quoted 3 lines

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is.

Understood I will in future not use previous because it is the _current_ code.

Quoted 4 lines

You may view it as a weakness (which
may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

I understand, git wants people to not explore the available change options and help them make logical decisions rather than pushing them with some unneeded commands.

Quoted 3 lines

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

Understood that makes sense because nevertheless
it is given that I am writing the changes for this patch only.

Quoted 3 lines

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

That makes total sense.

Quoted 3 lines

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Thank you so much for your guidance the advice should not trigger to those who have opted not to see.
My code might have misjudged this paradigm.

Quoted 1 line

Also "To move you" -> "To move your".

I thought I had fixed this typo. Seems like I didn't. I will remember to be more cautious next time.

Quoted 3 lines

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Absolutely correct this commit does narrow the users vision for exploring.

Quoted 3 lines

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

I don't want to sound like a programmed robot but I absolutely agree with the recommendations.

Quoted 2 lines

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thank you so much for your valuable guidance.
If it's possible I want some guidance over the questions written below, As it is well stated by you that the current
text is clear enough.
Should I still work on this PR from a purely GSoC perspective. Or should I start making my proposal or
still work on this PR until my micro project is merged? Because I have already shown I can navigate git project which was the goal of micro projects in the first place.

Quoted 101 lines · Junio C Hamano

Quoted 5 lines · Arsh Srivastava via GitGitGadget

When a branch switch fails due to local changes and
new users who are not familiar with the error message often get confused about how to move ahead and resolve the issue as the previous error message only suggests to commit or stash the changes but doesn't explain how to do that or what the next steps are.

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence.

Perhaps the early part can become a bit easier to read with punctuations, and cutting the sentence into two, e.g.,

When a branch switch fails due to local changes, new users who are unfamiliar with the error message often get confused about how to move ahead and resolve the issue.

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is. The _current_ message stops at hinting the commands to be used without giving wordy instructions that are best left to manuals. You may view it as a weakness (which may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

Quoted 4 lines

This patch enhances the error message with more specific instructions in a concise manner to help users understand how to resolve the issue and move their local changes safely to the other branch using stash.

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

By the way, the updated message seems much less concise than the original.

unpack-trees.c void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, · line 135

Quoted 9 lines

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"

unpack-trees.c +5 -2

-               "Please commit your changes or stash them before you switch branches.")
-           : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
+               "To move you local changes safely to the other branch,\n"
+               "Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge."
+               )
+           : _("Your local changes to the following files would be overwritten by checkout:\n%%s"
+               "Please commit your changes or stash them before you switch branches.");

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

The original was this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.")
: _("Your local changes to the following files would be overwritten by checkout:\n%%s");

Note that when advice is *NOT* enabled, we only gave

_("Your local changes to the following files would be overwritten by checkout:\n%%s");

without any "advise" in the output. That is what !advice_enabled() means.

The updated code does this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"To move you local changes safely to the other branch,\n"
"Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge." )
: _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.");

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Also "To move you" -> "To move your".

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

A more concise version might say:

Try 'git stash && git checkout <branch> && git stash pop' to carry your changes to the new branch, or commit your work before switching.

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thanks.

I understand, git wants people to not explore the available

Junio C Hamano <[email protected]> writes :

Quoted 3 lines

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence. Perhaps the early part can become a bit easier to read with

punctuations, and cutting the sentence into two, e.g.,

In my future commits I will remember to make it as easy to read as possible. With less punctuations and shorter sentences which will in turn make it more concise.

Quoted 3 lines

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is.

Understood I will in future not use previous because it is the _current_ code.

Quoted 4 lines

You may view it as a weakness (which
may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

I understand, git wants people to not explore the available change options and help them make logical decisions rather than pushing them with some unneeded commands.

Quoted 3 lines

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

Understood that makes sense because nevertheless
it is given that I am writing the changes for this patch only.

Quoted 3 lines

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

That makes total sense.

Quoted 3 lines

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Thank you so much for your guidance the advice should not trigger to those who have opted not to see.
My code might have misjudged this paradigm.

Quoted 1 line

Also "To move you" -> "To move your".

I thought I had fixed this typo. Seems like I didn't. I will remember to be more cautious next time.

Quoted 3 lines

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Absolutely correct this commit does narrow the users vision for exploring.

Quoted 3 lines

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

I don't want to sound like a programmed robot but I absolutely agree with the recommendations.

Quoted 2 lines

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thank you so much for your valuable guidance.
If it's possible I want some guidance over the questions written below, As it is well stated by you that the current
text is clear enough.
Should I still work on this PR from a purely GSoC perspective. Or should I start making my proposal or
still work on this PR until my micro project is merged? Because I have already shown I can navigate git project which was the goal of micro projects in the first place.

Quoted 101 lines · Junio C Hamano

Quoted 5 lines · Arsh Srivastava via GitGitGadget

When a branch switch fails due to local changes and
new users who are not familiar with the error message often get confused about how to move ahead and resolve the issue as the previous error message only suggests to commit or stash the changes but doesn't explain how to do that or what the next steps are.

The first paragraph is a bit of a run-on and has a misplaced "and"; I cannot quite read and understand this overly long single sentence.

Perhaps the early part can become a bit easier to read with punctuations, and cutting the sentence into two, e.g.,

When a branch switch fails due to local changes, new users who are unfamiliar with the error message often get confused about how to move ahead and resolve the issue.

Also it is misleading to say "previous" error message. We talk about the current code in the present tense, to highlight what the problem in the current code is. The _current_ message stops at hinting the commands to be used without giving wordy instructions that are best left to manuals. You may view it as a weakness (which may motivate this patch to be written). But I personally am not so sure that adding words to the existing message would necessarily make it more clear.

Quoted 4 lines

This patch enhances the error message with more specific instructions in a concise manner to help users understand how to resolve the issue and move their local changes safely to the other branch using stash.

As Documentation/SubmittingPatches says, let's instruct the code to "be like so" in imperative mood. E.g., "Enhance the error message..." instead of "This patch enhances...".

By the way, the updated message seems much less concise than the original.

unpack-trees.c void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, · line 135

Quoted 9 lines

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"

unpack-trees.c +5 -2

-               "Please commit your changes or stash them before you switch branches.")
-           : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
+               "To move you local changes safely to the other branch,\n"
+               "Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge."
+               )
+           : _("Your local changes to the following files would be overwritten by checkout:\n%%s"
+               "Please commit your changes or stash them before you switch branches.");

These were already overly long, but the updated one is way too long to be read on end-user's terminal. The source lines are overly long, too.

The original was this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.")
: _("Your local changes to the following files would be overwritten by checkout:\n%%s");

Note that when advice is *NOT* enabled, we only gave

_("Your local changes to the following files would be overwritten by checkout:\n%%s");

without any "advise" in the output. That is what !advice_enabled() means.

The updated code does this:

msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"To move you local changes safely to the other branch,\n"
"Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge." )
: _("Your local changes to the following files would be overwritten by checkout:\n%%s"
"Please commit your changes or stash them before you switch branches.");

to those users who decline the advice, we now show "Please commit...". That is not what !advice_enabled() should trigger, is it?

Also "To move you" -> "To move your".

Also the advice lost the other possiblity of first committing the work in progress on the original branch before switching, yet the new advice message is quite wordy.

Also, using "for safe merge" when the user is performing a "checkout" might be slightly confusing, even if 'stash pop' involves a merge under the hood.

A more concise version might say:

Try 'git stash && git checkout <branch> && git stash pop' to carry your changes to the new branch, or commit your work before switching.

But as I already said, I think the current text may already strike the right balance between being clear and being concise.

Thanks.

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Documentation/config/advice.adoc +0 -0

 	statusAheadBehind::
 		Shown when linkgit:git-status[1] computes the ahead/behind
 		counts for a local ref compared to its remote tracking ref,

advice.c +17 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
 	[ADVICE_SPARSE_INDEX_EXPANDED]			= { "sparseIndexExpanded" },
+	[ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
 	[ADVICE_STATUS_HINTS]				= { "statusHints" },
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
 			    "* Use \"git add --sparse <paths>\" to update the index\n"
 			    "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+	return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+		      "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+	fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+		      _("You can save your local changes before switching by running:\n"
+			"\n"

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Quoted 6 lines · Arsh Srivastava via GitGitGadget

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

Quoted 7 lines

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

Nit: Did you mean to add bullet point here? '>' is generally used to quote text. Perhaps use '-' or '*'.

[snip]

Quoted 6 lines

 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Hmm. Shouldn't there be changes which actually call the newly introduced function? Also shouldn't there be tests added?

Documentation/config/advice.adoc all advice messages. · line 133

Quoted 13 lines

Documentation/config/advice.adoc +5 -0

index 257db58918..8752e05636 100644
@@ -126,6 +126,11 @@ all advice messages.
              Shown when a sparse index is expanded to a full index, which is likely
              due to an unexpected set of files existing outside of the
              sparse-checkout.
+     stashBeforeCheckout::
+             Shown when the user attempts to switch branches but has
+             local modifications that would be overwritten by the
+             operation, to suggest using linkgit:git-stash[1] to
+             save changes before switching.

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this?

In one of my repos:

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

❯ echo "aldjf" >> LICENSE

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
(use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory)
modified: LICENSE

no changes added to commit (use "git add" and/or "git commit -a")

❯ git checkout 0-1-stable
error: Your local changes to the following files would be overwritten by checkout:
LICENSE
Please commit your changes or stash them before you switch branches. Aborting

So won't this simply be duplicating the same message?

Quoted 35 lines

Documentation/config/advice.adoc +0 -0

      statusAheadBehind::
              Shown when linkgit:git-status[1] computes the ahead/behind
              counts for a local ref compared to its remote tracking ref,

advice.c +17 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
      [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
      [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
      [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+     [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
      [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
      [ADVICE_STATUS_HINTS]                           = { "statusHints" },
      [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                          "* Use \"git add --sparse <paths>\" to update the index\n"
                          "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+     return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+                   "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+     fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+                   _("You can save your local changes before switching by running:\n"
+                     "\n"

advice.c void advise_on_moving_dirty_path(struct string_list *pathspec_list) · line 341

Quoted 10 lines

advice.c +10 -0

+                     "\tgit stash push\n"
+                     "\n"
+                     "Then restore them after switching with:\n"
+                     "\n"
+                     "\tgit stash pop\n"
+                     "\n"
+                     "Or to discard your local changes, use:\n"
+                     "\n"
+                     "\tgit checkout -- <file>"));
+}

This doesn't seem to be formatted with tabs.

It is used in "add.c", but not magically so. The function that you have introduced is the only site that uses the new advice, but the function is never called as far as I can see. So ultimately, the proposed change does not have any effect on the user-observable behaviour.

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Quoted 2 lines · Karthik Nayak

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Thank you for the detailed review. You are correct, the existing message in unpack-trees.c already handles this case and my patch duplicates it. I also acknowledge the other issues raised:

- The newly introduced function was never called anywhere in the codebase
- No tests were added
- The bullet points in the commit message used '>' instead of '-' or '*'
- The advice message was not formatted with tabs

Rather than duplicating the existing behaviour, I think the better approach would be to enhance the existing message in unpack-trees.c to also mention 'git checkout -m' as an option for users who want to carry their local changes over to the new branch, since the current message only says "commit or stash" without mentioning that option.

I will rework the patch in that direction and send a v4.

Signed-off-by Arsh Srivastava <[email protected]>

Quoted 125 lines · Karthik Nayak

Quoted 6 lines · Arsh Srivastava via GitGitGadget

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

Quoted 7 lines

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

Nit: Did you mean to add bullet point here? '>' is generally used to quote text. Perhaps use '-' or '*'.

[snip]

Quoted 6 lines

 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Hmm. Shouldn't there be changes which actually call the newly introduced function? Also shouldn't there be tests added?

Documentation/config/advice.adoc all advice messages. · line 133

Quoted 13 lines

Documentation/config/advice.adoc +5 -0

index 257db58918..8752e05636 100644
@@ -126,6 +126,11 @@ all advice messages.
              Shown when a sparse index is expanded to a full index, which is likely
              due to an unexpected set of files existing outside of the
              sparse-checkout.
+     stashBeforeCheckout::
+             Shown when the user attempts to switch branches but has
+             local modifications that would be overwritten by the
+             operation, to suggest using linkgit:git-stash[1] to
+             save changes before switching.

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this?

In one of my repos:

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

❯ echo "aldjf" >> LICENSE

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
(use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory)
modified: LICENSE

no changes added to commit (use "git add" and/or "git commit -a")

❯ git checkout 0-1-stable
error: Your local changes to the following files would be overwritten by checkout:
LICENSE
Please commit your changes or stash them before you switch branches. Aborting

So won't this simply be duplicating the same message?

Quoted 35 lines

Documentation/config/advice.adoc +0 -0

      statusAheadBehind::
              Shown when linkgit:git-status[1] computes the ahead/behind
              counts for a local ref compared to its remote tracking ref,

advice.c +17 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
      [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
      [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
      [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+     [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
      [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
      [ADVICE_STATUS_HINTS]                           = { "statusHints" },
      [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                          "* Use \"git add --sparse <paths>\" to update the index\n"
                          "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+     return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+                   "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+     fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+                   _("You can save your local changes before switching by running:\n"
+                     "\n"

advice.c void advise_on_moving_dirty_path(struct string_list *pathspec_list) · line 341

Quoted 10 lines

advice.c +10 -0

+                     "\tgit stash push\n"
+                     "\n"
+                     "Then restore them after switching with:\n"
+                     "\n"
+                     "\tgit stash pop\n"
+                     "\n"
+                     "Or to discard your local changes, use:\n"
+                     "\n"
+                     "\tgit checkout -- <file>"));
+}

This doesn't seem to be formatted with tabs.

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Quoted 4 lines · Patrick Steinhardt

It is used in "add.c", but not magically so. The function that you have introduced is the only site that uses the new advice, but the function is never called as far as I can see. So ultimately, the proposed change does not have any effect on the user-observable behaviour.

Thank you for the correction and for the bottom-posting reminder.

You are right. The function advise_on_checkout_dirty_files() is defined but never called anywhere, so the patch has no user-observable effect. I also looked into the existing behaviour more carefully and found that unpack-trees.c already handles this case and prints a message telling the user to commit or stash their changes before switching branches.

So the patch as written is both incomplete and duplicates existing behaviour. I will rework it in v3 to instead enhance the existing message in unpack-trees.c to also mention 'git checkout -m' for users who want to carry their local changes over to the new branch.

Use of Ai in patches and messages
Suggest the documentation for the same

Quoted 18 lines · Arsh Srivastava

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Quoted 4 lines · Patrick Steinhardt

It is used in "add.c", but not magically so. The function that you have introduced is the only site that uses the new advice, but the function is never called as far as I can see. So ultimately, the proposed change does not have any effect on the user-observable behaviour.

Thank you for the correction and for the bottom-posting reminder.

You are right. The function advise_on_checkout_dirty_files() is defined but never called anywhere, so the patch has no user-observable effect. I also looked into the existing behaviour more carefully and found that unpack-trees.c already handles this case and prints a message telling the user to commit or stash their changes before switching branches.

Quoted 5 lines

So the patch as written is both incomplete and duplicates existing behaviour. I will rework it in v3 to instead enhance the existing message in unpack-trees.c to also mention 'git checkout -m' for users who want to carry their local changes over to the new branch.

I must say that the patch itself and the responses seem like they're generated with LLMs. Our guidelines for AI usage within the Git project are laid out in 'Documentation/SubmittingPatches'.

For GSoC specifically, we have laid out our expectations here [1]. Please go through them.

[1]: https://git.github.io/General-Application-Information/

[snip]

It is used in "add.c", but not magically so. The function that you have introduced is the only site that uses the new advice, but the function is never called as far as I can see. So ultimately, the proposed change does not have any effect on the user-observable behaviour.

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Quoted 2 lines · Karthik Nayak

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Thank you for the detailed review. You are correct, the existing message in unpack-trees.c already handles this case and my patch duplicates it. I also acknowledge the other issues raised:

- The newly introduced function was never called anywhere in the codebase
- No tests were added
- The bullet points in the commit message used '>' instead of '-' or '*'
- The advice message was not formatted with tabs

Rather than duplicating the existing behaviour, I think the better approach would be to enhance the existing message in unpack-trees.c to also mention 'git checkout -m' as an option for users who want to carry their local changes over to the new branch, since the current message only says "commit or stash" without mentioning that option.

I will rework the patch in that direction and send a v4.

Signed-off-by Arsh Srivastava <[email protected]>

Quoted 125 lines · Karthik Nayak

Quoted 6 lines · Arsh Srivastava via GitGitGadget

From: Arsh Srivastava <[email protected]>

Add a new advice type ADVICE_STASH_BEFORE_CHECKOUT to guide users when they attempt to switch branches with local modifications that would be overwritten by the operation.

Quoted 7 lines

This includes:

Quoted 3 lines

New ADVICE_STASH_BEFORE_CHECKOUT enum value in advice.h Corresponding "stashBeforeCheckout" entry in advice_setting[] New advise_on_checkout_dirty_files() function that lists the

affected files and suggests using git stash push/pop

Quoted 1 line

Documentation entry in Documentation/config/advice.txt

Nit: Did you mean to add bullet point here? '>' is generally used to quote text. Perhaps use '-' or '*'.

[snip]

Quoted 6 lines

 Documentation/config/advice.adoc |  5 +++++
 advice.c                         | 27 +++++++++++++++++++++++++++
 advice.h                         |  2 ++
 3 files changed, 34 insertions(+)

Hmm. Shouldn't there be changes which actually call the newly introduced function? Also shouldn't there be tests added?

Documentation/config/advice.adoc all advice messages. · line 133

Quoted 13 lines

Documentation/config/advice.adoc +5 -0

index 257db58918..8752e05636 100644
@@ -126,6 +126,11 @@ all advice messages.
              Shown when a sparse index is expanded to a full index, which is likely
              due to an unexpected set of files existing outside of the
              sparse-checkout.
+     stashBeforeCheckout::
+             Shown when the user attempts to switch branches but has
+             local modifications that would be overwritten by the
+             operation, to suggest using linkgit:git-stash[1] to
+             save changes before switching.

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this?

In one of my repos:

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

❯ echo "aldjf" >> LICENSE

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
(use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory)
modified: LICENSE

no changes added to commit (use "git add" and/or "git commit -a")

❯ git checkout 0-1-stable
error: Your local changes to the following files would be overwritten by checkout:
LICENSE
Please commit your changes or stash them before you switch branches. Aborting

So won't this simply be duplicating the same message?

Quoted 35 lines

Documentation/config/advice.adoc +0 -0

      statusAheadBehind::
              Shown when linkgit:git-status[1] computes the ahead/behind
              counts for a local ref compared to its remote tracking ref,

advice.c +17 -0

index 0018501b7b..e1264f525c 100644
@@ -81,6 +81,7 @@ static struct {
      [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure" },
      [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks" },
      [ADVICE_SPARSE_INDEX_EXPANDED]                  = { "sparseIndexExpanded" },
+     [ADVICE_STASH_BEFORE_CHECKOUT] = { "stashBeforeCheckout" },
      [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning" },
      [ADVICE_STATUS_HINTS]                           = { "statusHints" },
      [ADVICE_STATUS_U_OPTION]                        = { "statusUoption" },

@@ -312,3 +313,29 @@ void advise_on_moving_dirty_path(struct string_list *pathspec_list)
                          "* Use \"git add --sparse <paths>\" to update the index\n"
                          "* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));
 }
+
+void advise_on_checkout_dirty_files(struct string_list *file_list)
+{
+    struct string_list_item *item;
+
+    if (!file_list->nr)
+     return;
+
+    fprintf(stderr, _("The following files have local modifications that would\n"
+                   "be overwritten by switching branches:\n"));
+    for_each_string_list_item(item, file_list)
+     fprintf(stderr, "\t%s\n", item->string);
+
+    advise_if_enabled(ADVICE_STASH_BEFORE_CHECKOUT,
+                   _("You can save your local changes before switching by running:\n"
+                     "\n"

advice.c void advise_on_moving_dirty_path(struct string_list *pathspec_list) · line 341

Quoted 10 lines

advice.c +10 -0

+                     "\tgit stash push\n"
+                     "\n"
+                     "Then restore them after switching with:\n"
+                     "\n"
+                     "\tgit stash pop\n"
+                     "\n"
+                     "Or to discard your local changes, use:\n"
+                     "\n"
+                     "\tgit checkout -- <file>"));
+}

This doesn't seem to be formatted with tabs.

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Karthik Nayak <[email protected]> writes:

Quoted 2 lines

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Thank you for the detailed review. You are correct, the existing message
...
I will rework the patch in that direction and send a v4.

Signed-off-by Arsh Srivastava <[email protected]>

Quoted 12 lines · Arsh Srivastava

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Quoted 2 lines · Karthik Nayak

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Thank you for the detailed review. You are correct, the existing message
...
I will rework the patch in that direction and send a v4.

Quoted 1 line

Signed-off-by Arsh Srivastava <[email protected]>

Just a comment by a bystander, but it confuses me quite a lot to see in-body "Subject:" and "Sign-off" in a message that is *not* a patch at all. What are you signing off with this signature?

Quoted 7 lines

Quoted 3 lines

I will rework the patch in that direction and send a v4.

Signed-off-by Arsh Srivastava <[email protected]>

Just a comment by a bystander, but it confuses me quite a lot to see in-body "Subject:" and "Sign-off" in a message that is *not* a patch at all. What are you signing off with this signature?

Tangentially, I know that b4 adds a "Sign-off" to the cover message.

Sign-off and Subject in body confuses

Quoted 5 lines · Arsh Srivastava

Subject: Re: [GSOC] advice: add stashBeforeCheckout advice for dirty branch switches

Karthik Nayak <[email protected]> writes:

Quoted 8 lines

Quoted 2 lines

Doesn't 'ADVICE_COMMIT_BEFORE_MERGE' already do this? So won't this simply be duplicating the same message?

Thank you for the detailed review. You are correct, the existing message
...
I will rework the patch in that direction and send a v4.

Signed-off-by Arsh Srivastava <[email protected]>

Just a comment by a bystander, but it confuses me quite a lot to see in-body "Subject:" and "Sign-off" in a message that is *not* a patch at all. What are you signing off with this signature?